-
Notifications
You must be signed in to change notification settings - Fork 5
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
init view #395
init view #395
Conversation
WalkthroughThis pull request reorganizes and extends the environment-related sections of the application. The changes include renaming the Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant ET as EnvironmentTabs Component
participant R as Router
participant EP as Environment Overview Page
participant DB as Database
U->>ET: Clicks on a navigation tab (e.g., Deployments)
ET->>ET: Determine active tab based on URL
ET->>R: Call router.push(new URL)
R->>EP: Load corresponding page content
EP->>DB: Fetch environment data and statistics
DB-->>EP: Return fetched data
EP-->>U: Render updated page view
sequenceDiagram
participant U as User
participant DP as Deployments Page
participant TRPC as Deployments API Router
participant DB as Database
U->>DP: Access the Deployments section
DP->>TRPC: Request deployment statistics (envId, workspaceSlug)
TRPC->>DB: Query deployment details and job statuses
DB-->>TRPC: Return deployment stats data
TRPC-->>DP: Provide statistics and version distribution
DP-->>U: Render deployments with updated metrics
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🧹 Nitpick comments (1)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/page.tsx (1)
445-479
: Placeholders for Deployments, Resources, and Policies
These placeholders are well-labeled, providing a clear location for future data. Ensure that you eventually integrate real, dynamic data for these tabs to keep the user experience consistent across the entire environment overview.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/page.tsx
(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{ts,tsx}`: **Note on Error Handling:** Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error...
**/*.{ts,tsx}
: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/page.tsx
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Lint
- GitHub Check: Typecheck
- GitHub Check: build (linux/amd64)
🔇 Additional comments (3)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/page.tsx (3)
80-81
: Handle zero division for deployment success calculation
Ifstats.deployments.total
is 0, the expression(stats.deployments.successful / stats.deployments.total)
will trigger a division by zero, resulting inInfinity
orNaN
. Consider a conditional check to avoid errors or skewed results.Would you like me to auto-generate a script that verifies if
stats.deployments.total
can be 0 in usage?
89-213
: Great use of UI components for environment overview
The descriptive headers and structured data grid are clear and user-friendly, promoting easy navigation.
387-440
: Flexible policy configuration
The policy settings code is well-structured. Ensure that future expansions (e.g., more or differentapprovalRequirement
values or dynamic release windows) can be easily supported. Also confirm that the static release window times from 2023 are intentional.
...eSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/page.tsx
Outdated
Show resolved
Hide resolved
export default function EnvironmentOverviewPage(props: { | ||
params: { | ||
workspaceSlug: string; | ||
systemSlug: string; | ||
environmentId: string; | ||
}>; | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Use the dynamic props to retrieve environment data
It appears we're not using the params
from props. Switching from a static environment object to dynamic data ensures that the environment details reflect real or fetched data.
If you'd like, we can fetch environment data for the environmentId
from an external API or server function. For example:
export default function EnvironmentOverviewPage(props: {
params: {
workspaceSlug: string;
systemSlug: string;
environmentId: string;
};
}) {
- // Sample static data
- const environmentData = { ... }
+ const environmentData = await fetchEnvironmentData(props.params.environmentId);
...
}
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
export default function EnvironmentOverviewPage(props: { | |
params: { | |
workspaceSlug: string; | |
systemSlug: string; | |
environmentId: string; | |
}>; | |
}; | |
export default async function EnvironmentOverviewPage(props: { | |
params: { | |
workspaceSlug: string; | |
systemSlug: string; | |
environmentId: string; | |
}; | |
}) { | |
const environmentData = await fetchEnvironmentData(props.params.environmentId); | |
// ... rest of your component code that uses environmentData | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 4
🧹 Nitpick comments (10)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/_resources/page.tsx (2)
6-8
: Consider updating function signature to align with other environment pages.The function signature includes the
workspaceSlug
parameter, but in other files likeresources/page.tsx
, this has been simplified to only includeenvironmentId
. For consistency, consider following the same pattern.export default async function ResourcesPage(props: { - params: Promise<{ workspaceSlug: string; environmentId: string }>; + params: Promise<{ environmentId: string }>; }) {
16-19
: Consider implementing consistent UI pattern.The UI pattern here (using a simple div with container and margin classes) differs from the Card-based pattern used in the updated
resources/page.tsx
file. For consistency across environment pages, consider implementing the Card-based UI pattern.return ( - <div className="container m-8 mx-auto"> - <EditFilterForm environment={environment} workspaceId={workspace.id} /> - </div> + <Card> + <CardHeader> + <CardTitle>Resources</CardTitle> + <CardDescription>Resources managed in this environment</CardDescription> + </CardHeader> + <CardContent> + <EditFilterForm environment={environment} workspaceId={workspace.id} /> + </CardContent> + </Card> );apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/policies/page.tsx (1)
3-7
: Missing error handling for non-existent environment.The function doesn't check if the environment exists after awaiting the params, unlike the previous implementation. While the error might be caught downstream in the PoliciesPageContent component, it's a good practice to handle this at the page level.
export default async function PoliciesPage(props: { params: Promise<{ environmentId: string }>; }) { const { environmentId } = await props.params; + const environment = await api.environment.byId(environmentId); + if (environment == null) notFound(); return <PoliciesPageContent environmentId={environmentId} />; }apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/_components/EnvironmentTabs.tsx (1)
39-45
: Consider optimizing tab change handler.The current implementation has potentially redundant state updates. The tab change handler sets the active tab after navigation occurs, which might lead to unnecessary renders.
const onTabChange = (value: string) => { + setActiveTab(value); if (value === "overview") router.push(overviewUrl); if (value === "deployments") router.push(deploymentsUrl); if (value === "resources") router.push(resourcesUrl); if (value === "policies") router.push(policiesUrl); - setActiveTab(value); };Additionally, consider using a switch statement or object lookup for better readability and maintainability:
const onTabChange = (value: string) => { setActiveTab(value); - if (value === "overview") router.push(overviewUrl); - if (value === "deployments") router.push(deploymentsUrl); - if (value === "resources") router.push(resourcesUrl); - if (value === "policies") router.push(policiesUrl); + const urlMap = { + overview: overviewUrl, + deployments: deploymentsUrl, + resources: resourcesUrl, + policies: policiesUrl + }; + router.push(urlMap[value as keyof typeof urlMap]); };apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/resources/page.tsx (1)
11-24
: Missing error handling for non-existent environment.Similar to the PoliciesPage component, this page doesn't verify if the environment exists before rendering the content. Consider adding error handling to prevent rendering with an invalid environment ID.
export default async function ResourcesPage(props: { params: Promise<{ environmentId: string }>; }) { const { environmentId } = await props.params; + const environment = await api.environment.byId(environmentId); + if (environment == null) notFound(); return ( <Card> <CardHeader> <CardTitle>Resources</CardTitle> <CardDescription>Resources managed in this environment</CardDescription> </CardHeader> <CardContent> - <ResourcesPageContent environmentId={environmentId} /> + <ResourcesPageContent environmentId={environment.id} /> </CardContent> </Card> ); }apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/overview/page.tsx (1)
433-485
: Remove or clarify large commented code blocks.
This extensive commented-out section may cause confusion and code bloat. If it’s not used soon, consider removing it or moving these references into documentation or separate feature-flagged code paths.apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/deployments/EnvironmentDeploymentsPageContent.tsx (2)
80-138
: Consider dynamic data fetching for deployments.
All deployment entries are static, which might become stale quickly. If these are placeholders, clarify in comments or plan for integration with a real API soon.Would you like me to provide a stub for an async data-fetching mechanism or open a new issue for future implementation?
306-311
: Leverage row selection logic for more user-friendly interactions.
You already haveonClick={() => setSelectedDeployment(deployment)}
on each row. Consider highlighting or focusing the row for clarity.<TableRow key={deployment.id} className="cursor-pointer border-b border-neutral-800/50 hover:bg-neutral-800/20" + style={{ outline: selectedDeployment?.id === deployment.id ? '2px solid #4f46e5' : 'none' }} onClick={() => setSelectedDeployment(deployment)} >apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/resources/ResourcesPageContent.tsx (2)
273-316
: Consider fallback logic for filter conditions.
WhenresourceFilter.operator
is "and" or "or", each condition is matched by exact string comparison. Ifvalue
is empty or the condition type is unrecognized, resources pass through. You might want to handle incomplete or unknown filter conditions more explicitly.
550-554
: Enhance accessibility in grid view.
Resource cards lack explicit interactive elements or ARIA attributes for accessibility. Consider adding focus, role, and label attributes so keyboard and screen-reader users can navigate effectively.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/_components/EnvironmentTabs.tsx
(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/_deployments/page.tsx
(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/_policies/page.tsx
(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/_resources/page.tsx
(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/deployments/DeploymentDetail.tsx
(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/deployments/EnvironmentDeploymentsPageContent.tsx
(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/deployments/page.tsx
(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/layout.tsx
(3 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/overview/page.tsx
(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/page.tsx
(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/policies/PoliciesPageContent.tsx
(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/policies/page.tsx
(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/resources/ResourcesPageContent.tsx
(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/resources/page.tsx
(1 hunks)apps/webservice/src/app/urls.ts
(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/page.tsx
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{ts,tsx}`: **Note on Error Handling:** Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error...
**/*.{ts,tsx}
: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
apps/webservice/src/app/urls.ts
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/deployments/page.tsx
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/_deployments/page.tsx
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/_policies/page.tsx
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/_resources/page.tsx
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/resources/page.tsx
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/policies/PoliciesPageContent.tsx
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/_components/EnvironmentTabs.tsx
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/deployments/DeploymentDetail.tsx
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/deployments/EnvironmentDeploymentsPageContent.tsx
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/resources/ResourcesPageContent.tsx
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/policies/page.tsx
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/layout.tsx
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/overview/page.tsx
🧬 Code Definitions (8)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/_deployments/page.tsx (1)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/deployments/page.tsx (1) (1)
DeploymentsPage
(11:26)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/_policies/page.tsx (1)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/policies/page.tsx (1) (1)
PoliciesPage
(3:8)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/_resources/page.tsx (2)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/resources/page.tsx (1) (1)
ResourcesPage
(11:26)apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/_resources/EditFilterForm.tsx (1) (1)
EditFilterForm
(113:214)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/resources/page.tsx (2)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/_resources/page.tsx (1) (1)
ResourcesPage
(6:21)apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/resources/ResourcesPageContent.tsx (1) (1)
ResourcesPageContent
(18:915)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/_components/EnvironmentTabs.tsx (1)
apps/webservice/src/app/urls.ts (1) (1)
urls
(194:194)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/deployments/EnvironmentDeploymentsPageContent.tsx (1)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/deployments/DeploymentDetail.tsx (1) (1)
DeploymentDetail
(13:646)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/policies/page.tsx (2)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/_policies/page.tsx (1) (1)
PoliciesPage
(3:13)apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/policies/PoliciesPageContent.tsx (1) (1)
PoliciesPageContent
(34:495)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/layout.tsx (2)
apps/webservice/src/app/urls.ts (1) (1)
urls
(194:194)apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/_components/EnvironmentTabs.tsx (1) (1)
EnvironmentTabs
(11:57)
🔇 Additional comments (18)
apps/webservice/src/app/urls.ts (1)
142-142
: Clean and consistent addition of the overview URL method.The new
overview
method follows the established pattern in the codebase and correctly utilizes the existingbuildUrl
function with the appropriate base segments.apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/_deployments/page.tsx (1)
1-13
: Implementation looks good, but clarify the purpose of this component.This component correctly handles the async parameters and renders the
DeploymentsCard
. The implementation is clean and straightforward.However, there's a similar component at
deployments/page.tsx
with a different Card-based UI structure. Could you clarify the relationship between these two files and why we need both implementations?apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/_policies/page.tsx (1)
1-13
: Simple redirection logic implemented correctly.The implementation correctly redirects to a specific policies URL, constructed using the provided parameters.
Note that this differs from the implementation in
policies/page.tsx
which renders aPoliciesPageContent
component instead of redirecting. It would be helpful to understand the intended purpose of each implementation.apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/deployments/page.tsx (1)
1-24
: UI enhancement with proper Card components.The changes improve the presentation by implementing a structured Card layout with appropriate semantic sections (header, title, description, content). The implementation correctly delegates the content rendering to the specialized
EnvironmentDeploymentsPageContent
component.The code maintains the same functionality while enhancing the UI, which is a good improvement.
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/_resources/page.tsx (1)
9-11
: Error handling pattern is solid.Good implementation of error handling using the
notFound()
function when expected data isn't available. This prevents the app from crashing and provides a better user experience.apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/_components/EnvironmentTabs.tsx (4)
11-17
: Good use of type-safe params extraction.The component correctly uses the
useParams
hook with TypeScript generics to ensure type safety for the URL parameters, which is a good practice.
18-25
: Clean URL construction with utility method.Using the
urls
utility for URL construction is a good practice as it centralizes URL generation logic, making it easier to maintain and update URLs across the application.
27-34
: Tab initialization logic is well-structured.The
getInitialTab
function effectively determines the initial active tab based on the current pathname, providing a good user experience by highlighting the correct tab when navigating directly to a specific URL.
47-56
: Semantic and accessible tab structure.The tab structure using
Tabs
,TabsList
, andTabsTrigger
components is semantically correct and likely provides good accessibility. The component correctly binds the active tab value and change handler.apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/resources/page.tsx (3)
1-7
: Well-structured imports for UI components.The imports are well-organized, grouping related UI components from the card library together, which improves code readability.
11-14
: Function signature simplification is a good improvement.The function now only requires the
environmentId
parameter, which simplifies the interface and reduces the need for fetching workspace data that may not be necessary for rendering resource information.
15-24
: Consistent card-based UI pattern.The implementation uses a clean, consistent card-based UI pattern with appropriate header and content sections, making it visually consistent with other parts of the application. The descriptive title and subtitle provide clear context for users.
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/policies/PoliciesPageContent.tsx (2)
1-458
: All-around solid component structure and descriptive tooltips.Your usage of tooltips, badges, and cards creates a clear hierarchy of environment policy details, providing a good user experience.
477-495
: Layout and styling look consistent.Beyond the noted release-interval mismatch, the rest of the UI elements here operate cleanly and match the overall design well.
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/layout.tsx (1)
42-87
: New layout structure is cohesive and user-friendly.The simplified approach with
<EnvironmentTabs>
and the top-levelPageHeader
suits the updated design by removing sidebar complexity and focusing on key environment details.apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/overview/page.tsx (1)
19-25
: Question the usage of a Promise type for props.
Declaringparams
as aPromise<{ ... }>
is unconventional and may cause issues if not properly awaited or resolved before rendering. Typically, you would accept a direct object or an async function that awaits the params before rendering.Are there other references in the codebase that ensure this promise is resolved before rendering? Consider verifying its usage in related routes or data-fetching logic.
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/deployments/EnvironmentDeploymentsPageContent.tsx (1)
140-154
:❓ Verification inconclusive
Check edge cases in time and duration formatting.
formatDuration
andformatTimeAgo
handle positive values but may behave unexpectedly with negative, zero, or extremely large numbers.Please ensure no negative or unintended values occur by verifying upstream data. Use a script or test suite to confirm correct usage:
🏁 Script executed:
#!/bin/bash # Searching for potential zero/negative usage of time values rg --case-sensitive 'formatDuration|formatTimeAgo' -A 5Length of output: 12654
ACTION: Verify and Align Edge Case Handling in Time Formatting
There appears to be an inconsistency in how edge cases are managed across similar functions:
- In
EnvironmentDeploymentsPageContent.tsx
,formatDuration
returns"—"
when the input isnull
and formats 0 as"0s"
. In contrast, inDeploymentPerformance.tsx
, the version offormatDuration
returns"N/A"
when given a falsy value.- Neither implementation explicitly guards against negative numbers or extremely large values. Similarly,
formatTimeAgo
may yield unexpected output if the supplied date is in the future.Recommendations:
- Upstream Verification: Ensure that the data passed to these functions is sanitized upstream, so that negative, future, or otherwise invalid values do not occur.
- Consistency Check: Consider unifying the behavior across components—for example, deciding whether 0 seconds should display as
"0s"
or"N/A"
, and ensuring similar guard clauses for negative values in bothformatDuration
andformatTimeAgo
.- Additional Guards: If necessary, add explicit checks to handle negative or overly large numbers, and safeguard against dates in the future in
formatTimeAgo
.apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/resources/ResourcesPageContent.tsx (1)
19-35
: Validate or remove default filter condition.
By default, the filter is set to"kind" = "Pod"
, which may undesirably hide non-Pod resources from the start. Make sure this is intended or consider a neutral default.
...aw)/systems/[systemSlug]/(raw)/environments/[environmentId]/policies/PoliciesPageContent.tsx
Outdated
Show resolved
Hide resolved
...lug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/layout.tsx
Show resolved
Hide resolved
Deployed {formatTimeAgo(deployment.deployedAt)} | ||
</p> | ||
</div> | ||
<div>{renderStatusBadge(deployment.status)}</div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing function references for status badge and time formatting.
You call formatTimeAgo(deployment.deployedAt)
at line 52 and renderStatusBadge(deployment.status)
at line 55, but neither is imported or defined. This will cause a runtime error.
One approach:
+function formatTimeAgo(date: Date) {
+ // Replace with actual logic or library call:
+ // e.g., from 'date-fns' or custom logic
+ return 'some time ago';
+}
+function renderStatusBadge(status: string) {
+ // Implement your own status -> badge logic
+ return <span>Status: {status}</span>;
+}
Then incorporate them into the component or import from a shared utility.
...pp)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/overview/page.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 6
🧹 Nitpick comments (7)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/deployments/_components/DeploymentDetail.tsx (3)
13-16
: Consider adding proper TypeScript typing for the deployment prop.The
deployment
prop is typed asany
, which loses type safety benefits. Consider creating a proper interface or type for the deployment object.+interface DeploymentProps { + id: string; + name: string; + status: "success" | "pending" | "failed" | "running"; + version: string; + deployedAt: Date; + duration: number | null; + resources: number; + initiatedBy: string; + successRate: number | null; +} export const DeploymentDetail: React.FC<{ - deployment: any; + deployment: DeploymentProps; onClose: () => void; }> = ({ deployment, onClose }) => {
20-46
: Consider extracting complex conditional class logic.The conditional class string construction is complex and repeated in multiple places. Consider extracting this pattern into a helper function for better maintainability.
+ const getStatusBackgroundClass = (status: string) => { + return status === "success" + ? "bg-green-500/10" + : status === "pending" + ? "bg-amber-500/10" + : status === "failed" + ? "bg-red-500/10" + : "bg-blue-500/10"; + }; <div - className={`-mx-6 -mt-6 mb-6 flex items-center justify-between px-6 py-4 ${ - deployment.status === "success" - ? "bg-green-500/10" - : deployment.status === "pending" - ? "bg-amber-500/10" - : deployment.status === "failed" - ? "bg-red-500/10" - : "bg-blue-500/10" - }`} + className={`-mx-6 -mt-6 mb-6 flex items-center justify-between px-6 py-4 ${getStatusBackgroundClass(deployment.status)}`} >
208-217
: Simplify complex class conditionals.Multiple class determination conditionals are similarly complex. Consider simplifying the determination of class names based on status.
<div className={`absolute left-0 top-1 h-5 w-5 rounded-full ${ - deployment.status === "success" - ? "border-2 border-green-500 bg-green-500/20" - : deployment.status === "failed" - ? "border-2 border-red-500 bg-red-500/20" - : deployment.status === "pending" - ? "border border-neutral-700 bg-neutral-800/80" - : "border-2 border-blue-500 bg-blue-500/20" + { + "success": "border-2 border-green-500 bg-green-500/20", + "failed": "border-2 border-red-500 bg-red-500/20", + "pending": "border border-neutral-700 bg-neutral-800/80", + "running": "border-2 border-blue-500 bg-blue-500/20" + }[deployment.status] || "border border-neutral-700 bg-neutral-800/80" } flex items-center justify-center`} >apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/deployments/EnvironmentDeploymentsPageContent.tsx (4)
173-237
: Add loading states for summary metrics.The summary cards display static data with no loading states. Consider adding loading states for these metrics when data is being fetched.
<div className="mb-6 grid grid-cols-1 gap-4 md:grid-cols-4"> + {isLoading ? ( + Array(4).fill(0).map((_, i) => ( + <div key={i} className="rounded-lg border border-neutral-800 bg-neutral-900/50 p-4"> + <div className="h-4 w-24 animate-pulse rounded bg-neutral-800"></div> + <div className="mt-2 h-8 w-16 animate-pulse rounded bg-neutral-800"></div> + </div> + )) + ) : (
239-273
: Add functionality to search and filter controls.The search and filter controls are not functional. Implement event handlers to filter the deployments based on user input.
<Input placeholder="Search deployments..." className="w-full pl-8 md:w-80" + onChange={(e) => { + const searchValue = e.target.value.toLowerCase(); + // Filter deployments by search value + }} />
305-356
: Add empty state for no deployments.There's no handling for when the deployments array is empty. Consider adding an empty state message for better user experience.
<TableBody> + {deployments.length === 0 ? ( + <TableRow> + <TableCell colSpan={8} className="py-8 text-center text-neutral-400"> + No deployments found + </TableCell> + </TableRow> + ) : ( {deployments.map((deployment) => ( // ...existing code ))} + )} </TableBody>
140-168
: Consider using date-fns for consistent formatting.The component defines its own time formatting functions (
formatDuration
andformatTimeAgo
) while the importedDeploymentDetail
component usesformatDuration
from date-fns. Consider using date-fns consistently for all time formatting.-const formatDuration = (seconds: number | null) => { - if (seconds === null) return "—"; - - if (seconds < 60) { - return `${seconds}s`; - } else if (seconds < 3600) { - const minutes = Math.floor(seconds / 60); - const remainingSeconds = seconds % 60; - return `${minutes}m ${remainingSeconds}s`; - } else { - const hours = Math.floor(seconds / 3600); - const minutes = Math.floor((seconds % 3600) / 60); - return `${hours}h ${minutes}m`; - } -}; +import { formatDuration as formatDurationFn, formatDistanceToNow } from "date-fns"; + +const formatDuration = (seconds: number | null) => { + if (seconds === null) return "—"; + + // Convert seconds to milliseconds for date-fns + return formatDurationFn({ seconds }); +}; + +const formatTimeAgo = (date: Date) => { + return formatDistanceToNow(date, { addSuffix: true }); +};
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/deployments/EnvironmentDeploymentsPageContent.tsx
(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/deployments/_components/DeploymentDetail.tsx
(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{ts,tsx}`: **Note on Error Handling:** Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error...
**/*.{ts,tsx}
: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/deployments/_components/DeploymentDetail.tsx
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/deployments/EnvironmentDeploymentsPageContent.tsx
🧬 Code Definitions (1)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/deployments/EnvironmentDeploymentsPageContent.tsx (1)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/deployments/_components/DeploymentDetail.tsx (1) (1)
DeploymentDetail
(13-646)
🔇 Additional comments (1)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/deployments/EnvironmentDeploymentsPageContent.tsx (1)
373-378
: Properly implement the DeploymentDetail component.Good integration of the DeploymentDetail component with proper state management.
).toLocaleTimeString()} | ||
] Creating deployment plan for {deployment.resources}{" "} | ||
resources... | ||
</p> | ||
<p className="text-neutral-400"> | ||
[ | ||
{new Date( | ||
deployment.deployedAt.getTime() + 62000, | ||
).toLocaleTimeString()} | ||
] Updating configuration... | ||
</p> | ||
<p className="text-neutral-400"> | ||
[ | ||
{new Date( | ||
deployment.deployedAt.getTime() + 95000, | ||
).toLocaleTimeString()} | ||
] Applying changes to resources... | ||
</p> | ||
{deployment.status === "success" ? ( | ||
<> | ||
<p className="text-neutral-400"> | ||
[ | ||
{new Date( | ||
deployment.deployedAt.getTime() + 145000, | ||
).toLocaleTimeString()} | ||
] Running post-deployment verification... | ||
</p> | ||
<p className="text-neutral-400"> | ||
[ | ||
{new Date( | ||
deployment.deployedAt.getTime() + 180000, | ||
).toLocaleTimeString()} | ||
] All health checks passed. | ||
</p> | ||
<p className="text-green-400"> | ||
[ | ||
{new Date( | ||
deployment.deployedAt.getTime() + 185000, | ||
).toLocaleTimeString()} | ||
] Deployment completed successfully! | ||
</p> | ||
</> | ||
) : deployment.status === "failed" ? ( | ||
<> | ||
<p className="text-neutral-400"> | ||
[ | ||
{new Date( | ||
deployment.deployedAt.getTime() + 110000, | ||
).toLocaleTimeString()} | ||
] Updating resource '{deployment.name}-1'... | ||
</p> | ||
<p className="text-red-400"> | ||
[ | ||
{new Date( | ||
deployment.deployedAt.getTime() + 125000, | ||
).toLocaleTimeString()} | ||
] Error: Failed to update resource '{deployment.name} | ||
-1'. | ||
</p> | ||
<p className="text-red-400"> | ||
[ | ||
{new Date( | ||
deployment.deployedAt.getTime() + 126000, | ||
).toLocaleTimeString()} | ||
] Error details: Configuration validation failed - | ||
insufficient permissions. | ||
</p> | ||
<p className="text-neutral-400"> | ||
[ | ||
{new Date( | ||
deployment.deployedAt.getTime() + 127000, | ||
).toLocaleTimeString()} | ||
] Rolling back changes... | ||
</p> | ||
<p className="text-red-400"> | ||
[ | ||
{new Date( | ||
deployment.deployedAt.getTime() + 135000, | ||
).toLocaleTimeString()} | ||
] Deployment failed. See detailed logs for more | ||
information. | ||
</p> | ||
</> | ||
) : ( | ||
<> | ||
<p className="text-blue-400"> | ||
[ | ||
{new Date( | ||
deployment.deployedAt.getTime() + 105000, | ||
).toLocaleTimeString()} | ||
] Currently updating resource {deployment.name}-3... | ||
</p> | ||
<p className="text-blue-400"> | ||
[{new Date().toLocaleTimeString()}] Deployment in | ||
progress (2/{deployment.resources} resources | ||
completed)... | ||
</p> | ||
</> | ||
)} | ||
</div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Consider using a real logging library or API endpoint.
The deployment logs section uses hardcoded mock data with calculated timestamps. In a real implementation, these should be fetched from an API.
...[systemSlug]/(raw)/environments/[environmentId]/deployments/_components/DeploymentDetail.tsx
Outdated
Show resolved
Hide resolved
...[systemSlug]/(raw)/environments/[environmentId]/deployments/_components/DeploymentDetail.tsx
Outdated
Show resolved
Hide resolved
...emSlug]/(raw)/environments/[environmentId]/deployments/EnvironmentDeploymentsPageContent.tsx
Outdated
Show resolved
Hide resolved
...emSlug]/(raw)/environments/[environmentId]/deployments/EnvironmentDeploymentsPageContent.tsx
Outdated
Show resolved
Hide resolved
...emSlug]/(raw)/environments/[environmentId]/deployments/EnvironmentDeploymentsPageContent.tsx
Outdated
Show resolved
Hide resolved
7a24e39
to
0d0b57c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
♻️ Duplicate comments (3)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/deployments/_components/DeploymentDetail.tsx (3)
20-20
: 🛠️ Refactor suggestionEnhance modal accessibility.
The modal implementation lacks critical accessibility features.
-<div className="fixed inset-0 z-50 flex items-center justify-center bg-black/60"> +<div + className="fixed inset-0 z-50 flex items-center justify-center bg-black/60" + role="dialog" + aria-modal="true" + onKeyDown={(e) => { + if (e.key === 'Escape') onClose(); + }} +>
52-53
:⚠️ Potential issueMissing helper function definition.
The
formatTimeAgo
function is used but not defined within this component or imported.+const formatTimeAgo = (date: Date) => { + const now = new Date(); + const diffInSeconds = Math.floor((now.getTime() - date.getTime()) / 1000); + + if (diffInSeconds < 60) return `${diffInSeconds} seconds ago`; + if (diffInSeconds < 3600) return `${Math.floor(diffInSeconds / 60)} minutes ago`; + if (diffInSeconds < 86400) return `${Math.floor(diffInSeconds / 3600)} hours ago`; + if (diffInSeconds < 2592000) return `${Math.floor(diffInSeconds / 86400)} days ago`; + return date.toLocaleDateString(); +}; <p className="text-sm text-neutral-400"> Deployed {formatTimeAgo(deployment.deployedAt)} </p>
55-55
:⚠️ Potential issueMissing helper function definition.
The
renderStatusBadge
function is used but not defined within this component or imported.+const renderStatusBadge = (status: string) => { + switch (status) { + case "success": + return ( + <Badge variant="outline" className="border-green-500/30 bg-green-500/10 text-green-400"> + Success + </Badge> + ); + case "failed": + return ( + <Badge variant="outline" className="border-red-500/30 bg-red-500/10 text-red-400"> + Failed + </Badge> + ); + case "pending": + return ( + <Badge variant="outline" className="border-amber-500/30 bg-amber-500/10 text-amber-400"> + Pending + </Badge> + ); + default: + return ( + <Badge variant="outline" className="border-blue-500/30 bg-blue-500/10 text-blue-400"> + In Progress + </Badge> + ); + } +}; <div>{renderStatusBadge(deployment.status)}</div>
🧹 Nitpick comments (8)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/policies/PoliciesPageContent.tsx (1)
37-63
: Prefer fetching policy data dynamically.
Currently,environmentPolicy
is hard-coded. For production use, consider fetching policy data from an API or configuration source so it remains accurate and maintainable over time.apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/deployments/_components/DeploymentDetail.tsx (1)
13-15
: Consider using a more specific type for the deployment prop.The component uses
any
for the deployment prop type, which loses type safety. Consider creating a proper interface or type for the deployment object.+interface Deployment { + id: string; + name: string; + status: "success" | "failed" | "pending" | "running"; + version: string; + deployedAt: Date; + duration: number; + resources: number; + initiatedBy: string; + successRate: number | null; +} -export const DeploymentDetail: React.FC<{ - deployment: any; - onClose: () => void; -}> = ({ deployment, onClose }) => { +export const DeploymentDetail: React.FC<{ + deployment: Deployment; + onClose: () => void; +}> = ({ deployment, onClose }) => {apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/_components/EnvironmentTabs.tsx (1)
39-45
: Consider refactoring the tab change handler for better maintainability.The
onTabChange
function uses sequentialif
statements to handle each tab value. This approach works, but could be refactored to be more maintainable.- const onTabChange = (value: string) => { - if (value === "overview") router.push(overviewUrl); - if (value === "deployments") router.push(deploymentsUrl); - if (value === "resources") router.push(resourcesUrl); - if (value === "policies") router.push(policiesUrl); - setActiveTab(value); - }; + const onTabChange = (value: string) => { + const urlMap: Record<string, string> = { + overview: overviewUrl, + deployments: deploymentsUrl, + resources: resourcesUrl, + policies: policiesUrl, + }; + + router.push(urlMap[value] || overviewUrl); + setActiveTab(value); + };apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/resources/ResourcesPageContent.tsx (5)
821-826
: Avoid direct DOM manipulation in React components.Using
document.getElementById
is not recommended in React. Use React refs instead for accessing DOM elements.- const typeSelect = document.getElementById( - "condition-type", - ) as HTMLSelectElement; - const valueSelect = document.getElementById( - "condition-value", - ) as HTMLSelectElement; + // At the top of your component: + const typeSelectRef = useRef<HTMLSelectElement>(null); + const valueSelectRef = useRef<HTMLSelectElement>(null); + + // And in the JSX where you render these elements: + <select + ref={typeSelectRef} + className="w-full rounded-md border border-neutral-700 bg-neutral-900 px-3 py-2 text-sm" + defaultValue="kind" + id="condition-type" + > + + <select + ref={valueSelectRef} + className="w-full rounded-md border border-neutral-700 bg-neutral-900 px-3 py-2 text-sm" + id="condition-value" + > + + // Then in your click handler: + const typeSelect = typeSelectRef.current; + const valueSelect = valueSelectRef.current;
38-39
: Remove ESLint disable comment by fixing the dependency issue.The ESLint disable comment for react-hooks/exhaustive-deps suggests there might be a dependency issue in a hook. Consider properly addressing the dependency issue rather than disabling the lint rule.
- // eslint-disable-next-line react-hooks/exhaustive-deps - const resources = [ + // Use useMemo to properly handle this value if it needs to be memoized + const resources = useMemo(() => [ // ...resource array definition - ]; + ], []); // Empty dependency array since this is static data
1-915
: Consider breaking down this large component into smaller ones.At over 900 lines, this component is quite large and could be challenging to maintain. Consider extracting logical sections into smaller, focused components:
- ResourceSummaryCards
- ResourceFilters
- ResourceGrid
- ResourceTable
- FilterEditorModal
This refactoring would improve maintainability and reusability of the individual parts.
277-291
: Add type definitions for conditions to improve code safety.The filter conditions are using
any
type which can lead to type safety issues. Define proper interfaces for your filter conditions.// Define these interfaces at the top of your file interface FilterCondition { type: 'kind' | 'provider' | 'status' | 'component'; operator: 'equals'; not: boolean; value: string; } interface ResourceFilter { type: 'comparison'; operator: 'and' | 'or'; not: boolean; conditions: FilterCondition[]; } // Then update your useState to use these types: const [resourceFilter, setResourceFilter] = useState<ResourceFilter>({ // ...existing implementation }); // And update the any types in your filter functions: resourceFilter.conditions.forEach((condition: FilterCondition) => { // ... }); // And in your OR filter: resourceFilter.conditions.some((condition: FilterCondition) => { // ... });Also applies to: 296-310
322-390
: Extract renderResourceCard to a separate component for better reusability.The
renderResourceCard
function is quite complex and would be better as a standalone component. This would improve separation of concerns and reusability.// Create a separate component const ResourceCard: React.FC<{ resource: Resource }> = ({ resource }) => { const statusColor = { healthy: "bg-green-500", degraded: "bg-amber-500", failed: "bg-red-500", updating: "bg-blue-500", unknown: "bg-neutral-500", }; return ( <div className="rounded-lg border border-neutral-800 bg-neutral-900/60 p-4 transition-all hover:border-neutral-700 hover:bg-neutral-900" > {/* Card content */} </div> ); }; // Then in your main component, replace the render call with: {filteredResources.map((resource) => ( <ResourceCard key={resource.id} resource={resource} /> ))}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/_components/EnvironmentTabs.tsx
(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/_deployments/page.tsx
(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/_policies/page.tsx
(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/_resources/page.tsx
(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/deployments/EnvironmentDeploymentsPageContent.tsx
(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/deployments/_components/DeploymentDetail.tsx
(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/deployments/page.tsx
(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/layout.tsx
(3 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/overview/page.tsx
(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/page.tsx
(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/policies/PoliciesPageContent.tsx
(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/policies/page.tsx
(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/resources/ResourcesPageContent.tsx
(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/resources/page.tsx
(1 hunks)apps/webservice/src/app/urls.ts
(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- apps/webservice/src/app/urls.ts
- apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/page.tsx
- apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/_deployments/page.tsx
- apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/_policies/page.tsx
- apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/deployments/EnvironmentDeploymentsPageContent.tsx
- apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/layout.tsx
- apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/overview/page.tsx
- apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/_resources/page.tsx
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{ts,tsx}`: **Note on Error Handling:** Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error...
**/*.{ts,tsx}
: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/deployments/page.tsx
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/policies/PoliciesPageContent.tsx
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/deployments/_components/DeploymentDetail.tsx
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/_components/EnvironmentTabs.tsx
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/resources/page.tsx
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/resources/ResourcesPageContent.tsx
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/policies/page.tsx
🧬 Code Definitions (4)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/deployments/page.tsx (2)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/_deployments/page.tsx (1) (1)
DeploymentsPage
(3-13)apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/deployments/EnvironmentDeploymentsPageContent.tsx (1) (1)
EnvironmentDeploymentsPageContent
(77-381)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/_components/EnvironmentTabs.tsx (1)
apps/webservice/src/app/urls.ts (1) (1)
urls
(196-196)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/resources/page.tsx (2)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/_resources/page.tsx (1) (1)
ResourcesPage
(6-21)apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/resources/ResourcesPageContent.tsx (1) (1)
ResourcesPageContent
(18-915)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/policies/page.tsx (2)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/_policies/page.tsx (1) (1)
PoliciesPage
(3-13)apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/policies/PoliciesPageContent.tsx (1) (1)
PoliciesPageContent
(34-495)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Typecheck
- GitHub Check: Lint
- GitHub Check: build (linux/amd64)
🔇 Additional comments (12)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/policies/page.tsx (1)
3-7
: Ensure parameter resolution reliability.
Returning<PoliciesPageContent environmentId={environmentId} />
works well. However, consider whether you need any fallback or error handling ifprops.params
fails to resolve (e.g., in edge cases). Otherwise, this is straightforward and looks good.apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/policies/PoliciesPageContent.tsx (1)
458-476
: Logic mismatch with "Release Interval".
This code displaysenvironmentPolicy.releaseWindows.length
under "Release Interval." However, the tooltip indicates it should show the minimum delay between releases, stored inminimumReleaseInterval
. This is an existing issue that still applies.Apply this diff to align the label and value correctly:
- {environmentPolicy.releaseWindows.length} + {formatDurationText(environmentPolicy.minimumReleaseInterval)}apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/deployments/page.tsx (3)
1-9
: Good use of UI component imports.The card components from the UI library are properly imported and utilized to create a structured layout.
16-24
: Well-structured card implementation.The card layout implementation provides a clear visual hierarchy with appropriate header and content sections.
22-22
: Proper component composition with environment ID prop.The
EnvironmentDeploymentsPageContent
component correctly receives theenvironmentId
prop extracted from the async params.apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/deployments/_components/DeploymentDetail.tsx (2)
273-399
: Consider using a real logging library or API endpoint.The deployment logs section uses hardcoded mock data with calculated timestamps. In a real implementation, these should be fetched from an API.
514-642
: Good implementation of contextual actions.The action buttons in the footer dynamically adapt based on the deployment status, which is a good UX pattern. Different actions are available for different states (failed, success, pending).
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/_components/EnvironmentTabs.tsx (1)
1-58
: LGTM! Well-implemented tab navigation component.The
EnvironmentTabs
component is well-structured and implements a clean tab-based navigation system. The URL construction is nicely centralized through theurls
utility, and the tab initialization logic is clear and straightforward.apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/resources/page.tsx (2)
11-13
: Function signature update improves simplicity.The updated signature only requires
environmentId
which aligns with the component's actual needs. This is a good simplification.
14-25
: Clean and concise component structure.The Card-based UI provides a clear and consistent layout for the resources page. Separation of concerns is well-implemented by extracting the content to a dedicated component.
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/resources/ResourcesPageContent.tsx (2)
18-35
: Well-structured component initialization with useState hooks.The component's state management is well-organized with clear initial values for view selection, filter editor visibility, and resource filtering.
269-316
: Well-implemented filtering logic with useMemo for performance.The filtering logic is well-structured and uses
useMemo
to prevent unnecessary recalculations on each render.
...)/systems/[systemSlug]/(raw)/environments/[environmentId]/resources/ResourcesPageContent.tsx
Outdated
Show resolved
Hide resolved
Co-authored-by: Justin Brooks <justin@wandb.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
🧹 Nitpick comments (5)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/overview/CopyEnvIdButton.tsx (1)
1-33
: Add error handling for clipboard operations.The component works well for copying environment IDs to the clipboard. However, clipboard operations can fail in some browsers or contexts (e.g., permissions not granted).
Consider adding error handling:
const copyEnvironmentId = () => { - navigator.clipboard.writeText(environmentId); - toast.success("Environment ID copied", { - description: environmentId, - duration: 2000, - }); + navigator.clipboard.writeText(environmentId) + .then(() => { + toast.success("Environment ID copied", { + description: environmentId, + duration: 2000, + }); + }) + .catch((error) => { + console.error("Failed to copy: ", error); + toast.error("Failed to copy environment ID", { + duration: 2000, + }); + }); };packages/api/src/router/environment-page/overview.ts (4)
1-3
: Consider narrower imports from lodash for tree-shaking.Currently importing the entire
_
object might increase bundle size. If you only need specific methods (e.g.,sumBy
), importing them individually can help reduce final bundle size.
34-40
: Clarify naming fordeployedStatuses
.Including failure statuses under
deployedStatuses
can be confusing. Consider renaming to something more descriptive likerelevantStatuses
ortrackedStatuses
to better reflect its broader usage.
91-108
: Consider indexing for better performance on status queries.Since the query aggregates by
job.status
and orders byjob.createdAt
, adding a composite index on(status, createdAt)
in thejob
table could improve performance when dealing with large volumes of deployments or resources.
169-189
: Consider pagination for large deployments.If an environment has a large number of deployments, mapping and aggregating each can become expensive. Consider adding pagination or limiting query scope to enhance performance and responsiveness.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/_components/EnvironmentTabs.tsx
(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/overview/CopyEnvIdButton.tsx
(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/overview/page.tsx
(1 hunks)packages/api/src/router/environment-page/environment-page.ts
(1 hunks)packages/api/src/router/environment-page/overview.ts
(1 hunks)packages/api/src/router/environment.ts
(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/_components/EnvironmentTabs.tsx
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{ts,tsx}`: **Note on Error Handling:** Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error...
**/*.{ts,tsx}
: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
packages/api/src/router/environment-page/environment-page.ts
packages/api/src/router/environment.ts
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/overview/CopyEnvIdButton.tsx
packages/api/src/router/environment-page/overview.ts
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/overview/page.tsx
🧬 Code Definitions (1)
packages/api/src/router/environment-page/environment-page.ts (1)
packages/api/src/router/environment-page/overview.ts (1) (1)
overviewRouter
(124-191)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Typecheck
- GitHub Check: Lint
- GitHub Check: build (linux/amd64)
🔇 Additional comments (6)
packages/api/src/router/environment-page/environment-page.ts (1)
1-6
: Clean router organization structure.The code follows a modular approach by creating a dedicated router for environment page functionality. The modular organization enhances maintainability by separating concerns.
packages/api/src/router/environment.ts (1)
42-42
: Good integration of the new page router.The environment router is properly extended with the new page router, maintaining a clean hierarchy and organization of API endpoints.
Also applies to: 49-49
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/overview/page.tsx (1)
37-40
: Guard against division by zero is implemented properly.The code includes a fallback to prevent division by zero by using
(stats.deployments.total || 1)
as the denominator. This is a good practice.packages/api/src/router/environment-page/overview.ts (3)
140-144
: Verify correct scope for deployments.The code fetches deployments by
systemId
instead of restricting them directly byenvironmentId
. If your intention is to retrieve only the deployments tied to this environment, consider filtering on the environment’s own ID or verifying it’s correct to use system-level deployments here.
59-90
:❓ Verification inconclusive
Validate ordering logic in subquery.
The
selectDistinctOn(...)
approach relies on the ordering byjob.createdAt
anddeploymentVersion.createdAt
. Confirm that this ordering always yields the desired “latest” job for each resource. If job creation time ties are possible, you may need additional sorting columns or constraints.
🏁 Script executed:
#!/bin/bash # Description: Check if the DB enforces a unique or consistent ordering for job creation times. # Example approach: Inspect constraints or indexes on the job and deploymentVersion tables # (within your DB migrations). If no constraints exist, consider adding tie-breakers. fd "migrations" --type f --exec cat {} | rg "constraint|index|job|deploymentVersion"Length of output: 86
Ordering Logic Verification for Latest Job Query
The current subquery orders by
job.createdAt
anddeploymentVersion.createdAt
to pick the latest job per resource. However, our verification script did not reveal any database-level constraints or indexes to ensure these timestamps are unique. Please manually verify that, under tie conditions (e.g., identical creation times), this ordering always returns the desired result. If tie scenarios are possible, consider adding additional sorting criteria (such as a unique identifier) to consistently determine the "latest" entry.
109-112
:❓ Verification inconclusive
Double-check negative case for
notDeployed
.
notDeployed = numResources - successful - failed - inProgress - pending
could become negative if unexpected counts arise (e.g., if a resource had multiple statuses in edge cases). While current logic might avoid double-counting, ensure that partial or invalid job statuses cannot cause an underflow.
🏁 Script executed:
#!/bin/bash # Description: Look for potential edge cases where multiple statuses could be assigned to the same resource. cat <<EOF > check_multi_status.py resources_with_multi_status = [] # Hypothetical check: We'll parse the job statuses for each resource on the same deployment # to see if they have more than one final status. This is just an example outline. print("No actual code to run here, but you can adapt for verifying multi-status edge cases.") EOF python check_multi_status.pyLength of output: 121
Review Action: Verify Underflow Handling in
notDeployed
CalculationThe current subtraction:
const notDeployed = numResources - successful - failed - inProgress - pending;could yield a negative value if some resources are accidentally assigned multiple statuses. Although our initial check (using a placeholder script) didn't reveal a concrete edge case, it's important to confirm that:
- Multiple statuses per resource are not possible. If they are, the calculation may underflow.
- A safeguard is in place. For example, consider using:
or add a similar guard to ensure thatconst notDeployed = Math.max(0, numResources - (successful + failed + inProgress + pending));notDeployed
never falls below zero.Please verify this logic against your domain's constraints and update the logic accordingly if overlapping statuses are possible.
...pp)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/overview/page.tsx
Outdated
Show resolved
Hide resolved
...pp)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/overview/page.tsx
Outdated
Show resolved
Hide resolved
.query(async ({ ctx, input }) => { | ||
const environment = await ctx.db | ||
.select() | ||
.from(SCHEMA.environment) | ||
.where(eq(SCHEMA.environment.id, input)) | ||
.then(takeFirst); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Handle case when environment is not found.
If no matching environment is found in the DB, environment
might be undefined
, and subsequent property accesses (e.g. environment.systemId
) can cause runtime errors. Consider early-returning a 404 or error message if environment
is null.
+ if (!environment) {
+ throw new TRPCError({ code: 'NOT_FOUND', message: 'Environment not found' });
+ }
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
.query(async ({ ctx, input }) => { | |
const environment = await ctx.db | |
.select() | |
.from(SCHEMA.environment) | |
.where(eq(SCHEMA.environment.id, input)) | |
.then(takeFirst); | |
.query(async ({ ctx, input }) => { | |
const environment = await ctx.db | |
.select() | |
.from(SCHEMA.environment) | |
.where(eq(SCHEMA.environment.id, input)) | |
.then(takeFirst); | |
if (!environment) { | |
throw new TRPCError({ code: 'NOT_FOUND', message: 'Environment not found' }); | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 6
🧹 Nitpick comments (8)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/overview/_components/CopyEnvIdButton.tsx (3)
12-18
: Consider adding error handling for clipboard operations.The
navigator.clipboard.writeText()
returns a Promise that should be handled to catch potential errors that might occur during clipboard operations, especially in environments with restricted permissions.- const copyEnvironmentId = () => { - navigator.clipboard.writeText(environmentId); - toast.success("Environment ID copied", { - description: environmentId, - duration: 2000, - }); - }; + const copyEnvironmentId = () => { + navigator.clipboard.writeText(environmentId) + .then(() => { + toast.success("Environment ID copied", { + description: environmentId, + duration: 2000, + }); + }) + .catch(err => { + toast.error("Failed to copy to clipboard", { + description: "Please try again or copy manually", + duration: 2000, + }); + }); + };
19-32
: Button implementation is good, consider enhancing accessibility.The button implementation prevents default behavior and has a title attribute for tooltips. However, adding an aria-label would improve accessibility for screen readers.
<Button variant="ghost" size="icon" className="h-5 w-5 rounded-full hover:bg-neutral-800/50" onClick={(e) => { e.preventDefault(); copyEnvironmentId(); }} title="Copy environment ID" + aria-label="Copy environment ID" >
1-33
: Consider adding a browser compatibility check for the Clipboard API.The Clipboard API might not be supported in all browsers. A fallback mechanism would improve reliability.
"use client"; import React from "react"; import { IconCopy } from "@tabler/icons-react"; import { Button } from "@ctrlplane/ui/button"; import { toast } from "@ctrlplane/ui/toast"; export const CopyEnvIdButton: React.FC<{ environmentId: string; }> = ({ environmentId }) => { + const isClipboardSupported = () => { + return !!navigator.clipboard; + }; + + const fallbackCopyToClipboard = (text: string) => { + // Create a temporary textarea element + const textArea = document.createElement("textarea"); + textArea.value = text; + + // Make it invisible + textArea.style.position = "fixed"; + textArea.style.opacity = "0"; + + document.body.appendChild(textArea); + textArea.select(); + + try { + // Execute the copy command + const successful = document.execCommand("copy"); + if (successful) { + toast.success("Environment ID copied", { + description: environmentId, + duration: 2000, + }); + } else { + throw new Error("Copy command failed"); + } + } catch (err) { + toast.error("Failed to copy to clipboard", { + description: "Please try again or copy manually", + duration: 2000, + }); + } finally { + document.body.removeChild(textArea); + } + }; const copyEnvironmentId = () => { + if (isClipboardSupported()) { navigator.clipboard.writeText(environmentId) + .then(() => { + toast.success("Environment ID copied", { + description: environmentId, + duration: 2000, + }); + }) + .catch(() => { + fallbackCopyToClipboard(environmentId); + }); + } else { + fallbackCopyToClipboard(environmentId); + } - toast.success("Environment ID copied", { - description: environmentId, - duration: 2000, - }); }; // Rest of the component remains the sameThis implementation provides a fallback using the older
document.execCommand("copy")
method which has broader browser support.apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/overview/_components/DeploymentTelemetryTable.tsx (5)
19-25
: Consider defining colors with semantic meaningThe list of colors is currently defined without context. Consider using semantic naming to improve readability and maintainability.
-const colors = [ - "bg-green-500", - "bg-blue-500", - "bg-red-500", - "bg-purple-500", - "bg-amber-500", -]; +const versionColors = { + current: "bg-green-500", + previous: "bg-blue-500", + outdated: "bg-red-500", + experimental: "bg-purple-500", + unknown: "bg-amber-500", +}; + +// When version type isn't known, fall back to this array +const fallbackColors = Object.values(versionColors);
40-46
: Consider improving key usage in map functionUsing index as a key in React's map function can lead to rendering issues when the list changes. Try to use a more stable identifier if available.
-{Object.values(versionDistro).map(({ percentage }, index) => ( +{Object.entries(versionDistro).map(([version, { percentage }], index) => ( <div - key={index} + key={version} className={`h-full ${colors[index % colors.length]}`} style={{ width: `${percentage * 100}%` }} /> ))}
117-149
: Add empty state handling for deployments listThe component doesn't handle the case when no deployments are available. Consider adding an empty state message.
export const DeploymentTelemetryTable: React.FC<{ deployments: SCHEMA.Deployment[]; }> = ({ deployments }) => { return ( <div> <h4 className="mb-3 text-sm font-medium text-neutral-300"> Deployment Versions </h4> + {deployments.length === 0 ? ( + <div className="flex h-32 items-center justify-center rounded-lg border border-neutral-800/50 bg-neutral-900/50"> + <p className="text-sm text-neutral-400">No deployments found</p> + </div> + ) : ( <div className="overflow-hidden rounded-lg border border-neutral-800/50"> <Table> <TableHeader> <TableRow className="border-b border-neutral-800/50 hover:bg-transparent"> <TableHead className="w-[200px] py-3 font-medium text-neutral-400"> Deployments </TableHead> <TableHead className="w-[300px] py-3 font-medium text-neutral-400"> Current Distribution </TableHead> <TableHead className="w-[150px] py-3 font-medium text-neutral-400"> Desired Version </TableHead> <TableHead className="py-3 text-right font-medium text-neutral-400"> Deployment Status </TableHead> </TableRow> </TableHeader> <TableBody> {deployments.map((deployment) => ( <DeploymentRow key={deployment.id} deployment={deployment} /> ))} </TableBody> </Table> </div> + )} </div> ); };
67-73
: Consider making deployment status indicator dynamicThe green indicator dot appears to be hardcoded, but should likely reflect the actual deployment status.
<div className="flex items-center gap-2"> - <div className="h-2 w-2 rounded-full bg-green-500"></div> + <div className={`h-2 w-2 rounded-full ${ + deployment.status === "deployed" ? "bg-green-500" : + deployment.status === "pending" ? "bg-amber-500" : + deployment.status === "failed" ? "bg-red-500" : + "bg-neutral-500" + }`}></div> <span className="text-sm text-neutral-200">{deployment.name}</span> <span className="text-xs text-neutral-400"> ({isLoading ? "-" : resourceCount}) </span> </div>
54-63
: Consider extracting telemetry data transformation to a utility functionTo improve code maintainability, consider extracting the telemetry data transformation logic to a separate utility function.
For example, create a utility function:
const processTelemetryData = (telemetry: any | undefined, isLoading: boolean) => { return { resourceCount: telemetry?.resourceCount ?? 0, versionDistro: telemetry?.versionDistro ?? {}, isLoading, isEmpty: Object.entries(telemetry?.versionDistro ?? {}).length === 0 }; };Then use it in your component:
- const resourceCount = telemetry?.resourceCount ?? 0; - const versionDistro = telemetry?.versionDistro ?? {}; + const { resourceCount, versionDistro, isEmpty } = processTelemetryData(telemetry, isLoading);This makes the component more readable and the data transformation logic more testable.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/overview/_components/CopyEnvIdButton.tsx
(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/overview/_components/DeploymentTelemetryTable.tsx
(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/overview/page.tsx
(1 hunks)packages/api/src/router/environment-page/environment-page.ts
(1 hunks)packages/api/src/router/environment-page/overview/get-deployment-stats.ts
(1 hunks)packages/api/src/router/environment-page/overview/get-version-distro.ts
(1 hunks)packages/api/src/router/environment-page/overview/router.ts
(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/api/src/router/environment-page/environment-page.ts
- apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/overview/page.tsx
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{ts,tsx}`: **Note on Error Handling:** Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error...
**/*.{ts,tsx}
: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/overview/_components/CopyEnvIdButton.tsx
packages/api/src/router/environment-page/overview/get-deployment-stats.ts
packages/api/src/router/environment-page/overview/get-version-distro.ts
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/overview/_components/DeploymentTelemetryTable.tsx
packages/api/src/router/environment-page/overview/router.ts
🧬 Code Definitions (1)
packages/api/src/router/environment-page/overview/router.ts (2)
packages/api/src/router/environment-page/overview/get-deployment-stats.ts (1) (1)
getDeploymentStats
(27-108)packages/api/src/router/environment-page/overview/get-version-distro.ts (1) (1)
getVersionDistro
(8-59)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build (linux/amd64)
- GitHub Check: Typecheck
- GitHub Check: Lint
🔇 Additional comments (8)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/overview/_components/CopyEnvIdButton.tsx (2)
1-8
: Imports and dependencies look good!The component correctly uses "use client" directive and imports necessary dependencies.
9-11
: Props type definition is clear and appropriate.The component accepts a single string prop for the environment ID, which is properly typed.
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/overview/_components/DeploymentTelemetryTable.tsx (1)
27-49
: Well-implemented visualization component with proper state handlingThe DistroBar component handles loading and empty states appropriately, providing a clean visual representation of version distribution.
packages/api/src/router/environment-page/overview/get-deployment-stats.ts (2)
8-25
: Status arrays look appropriate.The definitions of
failureStatuses
,pendingStatuses
, and the combineddeployedStatuses
array are clear and typed. They provide a concise way to track relevant job states.
95-107
: Overall structure is well-organized.The final return object is straightforward, clearly partitioning job status counts and the number of non-deployed resources. Nice approach in giving a holistic view of deployment stats.
packages/api/src/router/environment-page/overview/get-version-distro.ts (1)
14-35
: Good distinct-on pattern for retrieving latest versions.Using
.selectDistinctOn
provides a clean approach to filtering the latest version tag per resource based on job creation time. This subquery construction is efficient for pulling the most recent data.packages/api/src/router/environment-page/overview/router.ts (2)
39-51
: Graceful short-circuit for null resource filter.Returning default values when
environment.resourceFilter
is null avoids unnecessary computation. This is a neat early exit pattern.
120-120
: Duplicate environment check needed for telemetry path.Similar to the check above, ensure
environment
is notundefined
prior to referencingenvironment.resourceFilter
. Otherwise, a missing environment record may cause a runtime error here as well.
...emSlug]/(raw)/environments/[environmentId]/overview/_components/DeploymentTelemetryTable.tsx
Outdated
Show resolved
Hide resolved
...emSlug]/(raw)/environments/[environmentId]/overview/_components/DeploymentTelemetryTable.tsx
Show resolved
Hide resolved
...emSlug]/(raw)/environments/[environmentId]/overview/_components/DeploymentTelemetryTable.tsx
Outdated
Show resolved
Hide resolved
const statsByJobStatus = await db | ||
.select({ | ||
successful: count( | ||
sql`CASE WHEN ${latestJobsPerResourceAndDeploymentSubquery.status} = ${JobStatus.Successful} THEN 1 ELSE NULL END`, | ||
), | ||
inProgress: count( | ||
sql`CASE WHEN ${latestJobsPerResourceAndDeploymentSubquery.status} = ${JobStatus.InProgress} THEN 1 ELSE NULL END`, | ||
), | ||
pending: count( | ||
sql`CASE WHEN ${latestJobsPerResourceAndDeploymentSubquery.status} IN (${sql.raw(pendingStatuses.map((s) => `'${s}'`).join(", "))}) THEN 1 ELSE NULL END`, | ||
), | ||
failed: count( | ||
sql`CASE WHEN ${latestJobsPerResourceAndDeploymentSubquery.status} IN (${sql.raw(failureStatuses.map((s) => `'${s}'`).join(", "))}) THEN 1 ELSE NULL END`, | ||
), | ||
}) | ||
.from(latestJobsPerResourceAndDeploymentSubquery) | ||
.then(takeFirst); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prevent potential runtime error if no rows are returned.
When using .then(takeFirst)
, if the subquery returns zero rows, statsByJobStatus
will be undefined
. The subsequent object destructuring could throw an error. Consider applying a fallback or guard statement to handle the case when no rows exist:
- .then(takeFirst);
+ .then((row) => row ?? { successful: 0, inProgress: 0, pending: 0, failed: 0 });
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
const statsByJobStatus = await db | |
.select({ | |
successful: count( | |
sql`CASE WHEN ${latestJobsPerResourceAndDeploymentSubquery.status} = ${JobStatus.Successful} THEN 1 ELSE NULL END`, | |
), | |
inProgress: count( | |
sql`CASE WHEN ${latestJobsPerResourceAndDeploymentSubquery.status} = ${JobStatus.InProgress} THEN 1 ELSE NULL END`, | |
), | |
pending: count( | |
sql`CASE WHEN ${latestJobsPerResourceAndDeploymentSubquery.status} IN (${sql.raw(pendingStatuses.map((s) => `'${s}'`).join(", "))}) THEN 1 ELSE NULL END`, | |
), | |
failed: count( | |
sql`CASE WHEN ${latestJobsPerResourceAndDeploymentSubquery.status} IN (${sql.raw(failureStatuses.map((s) => `'${s}'`).join(", "))}) THEN 1 ELSE NULL END`, | |
), | |
}) | |
.from(latestJobsPerResourceAndDeploymentSubquery) | |
.then(takeFirst); | |
const statsByJobStatus = await db | |
.select({ | |
successful: count( | |
sql`CASE WHEN ${latestJobsPerResourceAndDeploymentSubquery.status} = ${JobStatus.Successful} THEN 1 ELSE NULL END`, | |
), | |
inProgress: count( | |
sql`CASE WHEN ${latestJobsPerResourceAndDeploymentSubquery.status} = ${JobStatus.InProgress} THEN 1 ELSE NULL END`, | |
), | |
pending: count( | |
sql`CASE WHEN ${latestJobsPerResourceAndDeploymentSubquery.status} IN (${sql.raw(pendingStatuses.map((s) => `'${s}'`).join(", "))}) THEN 1 ELSE NULL END`, | |
), | |
failed: count( | |
sql`CASE WHEN ${latestJobsPerResourceAndDeploymentSubquery.status} IN (${sql.raw(failureStatuses.map((s) => `'${s}'`).join(", "))}) THEN 1 ELSE NULL END`, | |
), | |
}) | |
.from(latestJobsPerResourceAndDeploymentSubquery) | |
.then((row) => row ?? { successful: 0, inProgress: 0, pending: 0, failed: 0 }); |
const total = _.sumBy(versionCounts, (v) => v.count); | ||
|
||
return Object.fromEntries( | ||
versionCounts.map((v) => [ | ||
v.tag, | ||
{ count: v.count, percentage: v.count / total }, | ||
]), | ||
); | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid division by zero when total is empty.
If no version rows are found, total
becomes zero, resulting in a potential NaN
or Infinity when calculating (v.count / total)
. Safeguard this calculation with a conditional check:
- versionCounts.map((v) => [
- v.tag,
- { count: v.count, percentage: v.count / total },
- ]),
+ versionCounts.map((v) => {
+ const percentage = total > 0 ? v.count / total : 0;
+ return [
+ v.tag,
+ { count: v.count, percentage },
+ ];
+ }),
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
const total = _.sumBy(versionCounts, (v) => v.count); | |
return Object.fromEntries( | |
versionCounts.map((v) => [ | |
v.tag, | |
{ count: v.count, percentage: v.count / total }, | |
]), | |
); | |
}; | |
const total = _.sumBy(versionCounts, (v) => v.count); | |
return Object.fromEntries( | |
- versionCounts.map((v) => [ | |
- v.tag, | |
- { count: v.count, percentage: v.count / total }, | |
- ]), | |
+ versionCounts.map((v) => { | |
+ const percentage = total > 0 ? v.count / total : 0; | |
+ return [ | |
+ v.tag, | |
+ { count: v.count, percentage }, | |
+ ]; | |
+ }), | |
); | |
}; |
const environment = await ctx.db | ||
.select() | ||
.from(SCHEMA.environment) | ||
.where(eq(SCHEMA.environment.id, input)) | ||
.then(takeFirst); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add check for a non-existent environment.
If the ctx.db
query returns no rows, environment
will be undefined
. Accessing environment.systemId
or environment.resourceFilter
would throw an error. Ensure the code verifies that environment
is defined before proceeding:
- .then(takeFirst);
+ .then((env) => {
+ if (!env) {
+ throw new Error(`Environment with id: ${input} not found`);
+ }
+ return env;
+ });
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
const environment = await ctx.db | |
.select() | |
.from(SCHEMA.environment) | |
.where(eq(SCHEMA.environment.id, input)) | |
.then(takeFirst); | |
const environment = await ctx.db | |
.select() | |
.from(SCHEMA.environment) | |
.where(eq(SCHEMA.environment.id, input)) | |
.then((env) => { | |
if (!env) { | |
throw new Error(`Environment with id: ${input} not found`); | |
} | |
return env; | |
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
♻️ Duplicate comments (2)
packages/api/src/router/environment-page/overview/router.ts (2)
28-33
:⚠️ Potential issueAdd check for a non-existent environment.
If the
ctx.db
query returns no rows,environment
will beundefined
. Accessingenvironment.systemId
orenvironment.resourceFilter
would throw an error. Ensure the code verifies thatenvironment
is defined before proceeding.- .then(takeFirst); + .then((env) => { + if (!env) { + throw new Error(`Environment with id: ${input} not found`); + } + return env; + });
103-108
:⚠️ Potential issueAdd validation for environment existence.
Similar to the previous issue, ensure that the environment exists before accessing its properties.
- .then(takeFirst); + .then((env) => { + if (!env) { + throw new Error(`Environment with id: ${environmentId} not found`); + } + return env; + });
🧹 Nitpick comments (1)
packages/api/src/router/environment-page/overview/version-distro.ts (1)
16-67
: Consider handling potential division by zero.When calculating percentages, verify that the
total
is not zero to prevent potential division by zero errors.const total = _.sumBy(versionCounts, (v) => v.count); return Object.fromEntries( versionCounts.map((v) => [ v.tag, - { count: v.count, percentage: v.count / total }, + { count: v.count, percentage: total > 0 ? v.count / total : 0 }, ]), );
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/api/src/router/environment-page/overview/deployment-stats.ts
(1 hunks)packages/api/src/router/environment-page/overview/router.ts
(1 hunks)packages/api/src/router/environment-page/overview/version-distro.ts
(1 hunks)packages/api/src/router/environment-page/router.ts
(1 hunks)packages/api/src/router/environment.ts
(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/api/src/router/environment.ts
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{ts,tsx}`: **Note on Error Handling:** Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error...
**/*.{ts,tsx}
: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
packages/api/src/router/environment-page/router.ts
packages/api/src/router/environment-page/overview/version-distro.ts
packages/api/src/router/environment-page/overview/deployment-stats.ts
packages/api/src/router/environment-page/overview/router.ts
🧬 Code Definitions (1)
packages/api/src/router/environment-page/overview/router.ts (2)
packages/api/src/router/environment-page/overview/deployment-stats.ts (1) (1)
getDeploymentStats
(36-117)packages/api/src/router/environment-page/overview/version-distro.ts (1) (1)
getVersionDistro
(16-67)
🔇 Additional comments (4)
packages/api/src/router/environment-page/router.ts (1)
1-6
: LGTM! Clear and well-structured router implementation.The implementation creates a clean hierarchical router structure by incorporating the
overviewRouter
as a sub-route, following good modularization practices.packages/api/src/router/environment-page/overview/deployment-stats.ts (1)
36-117
: LGTM! Well-structured query implementation with comprehensive statistics.The implementation efficiently retrieves deployment statistics with appropriate filtering and aggregation. The calculation of various deployment status counts is done in a clean and maintainable way.
packages/api/src/router/environment-page/overview/router.ts (2)
120-130
: LGTM! Good conditional handling and resource filtering.The implementation appropriately handles the case where
resourceFilter
is null and creates a well-structured resource selector by combining environment and deployment filters.
131-150
: LGTM! Effective resource querying and version distribution calculation.The implementation efficiently retrieves resources matching the specified criteria and properly calculates version distribution for the resources.
const deploymentPromise = ctx.db | ||
.select() | ||
.from(SCHEMA.deployment) | ||
.where(eq(SCHEMA.deployment.id, deploymentId)) | ||
.then(takeFirst); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Validate deployment existence.
Check that the deployment exists before accessing its properties to prevent runtime errors.
- .then(takeFirst);
+ .then((deployment) => {
+ if (!deployment) {
+ throw new Error(`Deployment with id: ${deploymentId} not found`);
+ }
+ return deployment;
+ });
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
const deploymentPromise = ctx.db | |
.select() | |
.from(SCHEMA.deployment) | |
.where(eq(SCHEMA.deployment.id, deploymentId)) | |
.then(takeFirst); | |
const deploymentPromise = ctx.db | |
.select() | |
.from(SCHEMA.deployment) | |
.where(eq(SCHEMA.deployment.id, deploymentId)) | |
.then((deployment) => { | |
if (!deployment) { | |
throw new Error(`Deployment with id: ${deploymentId} not found`); | |
} | |
return deployment; | |
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
apps/webservice/src/app/[workspaceSlug]/(app)/_components/policy/drawer/EnvironmentPolicyDrawer.tsx (1)
73-87
: 💡 Verification agent🧩 Analysis chain
Enhanced URL parameter management
Allowing the
setEnvironmentPolicyId
function to also update thetab
parameter in the URL is a good improvement. Before merging, confirm that thetab
value is always valid (i.e., it matches the enum). A simple validation check can ensure the app won’t redirect to an invalid state.You can verify usage with:
Then inspect each call site to confirm it passes a valid
EnvironmentPolicyDrawerTab
.--- 🏁 Script executed: ```shell #!/bin/bash rg "setEnvironmentPolicyId"
Length of output: 1289
Action Required: Validate
tab
Value in URL Parameter UpdatesGreat job integrating the URL parameter management improvements. However, before merging, please ensure that the
tab
parameter is validated against the allowed values (i.e., theEnvironmentPolicyDrawerTab
enum). This will prevent the application from redirecting to an invalid state if an unexpected value is provided. Here are the key points:
- In
apps/webservice/src/app/[workspaceSlug]/(app)/_components/policy/drawer/EnvironmentPolicyDrawer.tsx
(lines 73–87), consider adding an explicit check such as:if (tab != null && !Object.values(EnvironmentPolicyDrawerTab).includes(tab)) { console.error('Invalid tab value:', tab); return; }- The grep output confirms that all call sites (including those in
EnvFlowBuilder.tsx
andPoliciesPageContent.tsx
) pass a value or leave thetab
parameter undefined. However, an extra guard insetEnvironmentPolicyId
will help ensure stability even if an invalid value is inadvertently passed or introduced in the future.Please address this validation before merging.
🧹 Nitpick comments (2)
apps/webservice/src/app/[workspaceSlug]/(app)/_components/environment/drawer/EnvironmentDrawer.tsx (1)
101-101
: Potential naming confusionCurrently, the variable name
isUsingOverridePolicy
is being assigned toenvironment?.policy.isDefaultPolicy
. This feels inverted and may cause confusion, as "override policy" usually suggests the opposite of "default policy." Consider renaming the variable to something likeisUsingDefaultPolicy
for clarity and consistency.- const isUsingOverridePolicy = environment?.policy.isDefaultPolicy ?? false; + const isUsingDefaultPolicy = environment?.policy.isDefaultPolicy ?? false;apps/webservice/src/app/[workspaceSlug]/(app)/_components/policy/drawer/EnvironmentPolicyDrawer.tsx (1)
66-66
: Minor improvement: Provide a more descriptive export nameExporting
param
is helpful for reusability. However, for better clarity, consider renaming it toenvironmentPolicyParam
to be self-documenting.- export const param = "environment_policy_id"; + export const environmentPolicyParam = "environment_policy_id";
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/policies/PoliciesPageContent.tsx
(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/policies/page.tsx
(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/_components/environment/drawer/EnvironmentDrawer.tsx
(2 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/_components/policy/drawer/EnvironmentPolicyDrawer.tsx
(2 hunks)apps/webservice/src/app/urls.ts
(2 hunks)packages/api/src/router/environment.ts
(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/webservice/src/app/urls.ts
- packages/api/src/router/environment.ts
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{ts,tsx}`: **Note on Error Handling:** Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error...
**/*.{ts,tsx}
: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
apps/webservice/src/app/[workspaceSlug]/(app)/_components/environment/drawer/EnvironmentDrawer.tsx
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/policies/page.tsx
apps/webservice/src/app/[workspaceSlug]/(app)/_components/policy/drawer/EnvironmentPolicyDrawer.tsx
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/policies/PoliciesPageContent.tsx
🧠 Learnings (1)
apps/webservice/src/app/[workspaceSlug]/(app)/_components/policy/drawer/EnvironmentPolicyDrawer.tsx (1)
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#188
File: apps/webservice/src/app/[workspaceSlug]/_components/environment-drawer/EnvironmentDrawer.tsx:139-154
Timestamp: 2025-03-19T18:38:39.246Z
Learning: In `EnvironmentDrawer.tsx`, for the `EnvironmentDrawer` component, when there are only a few `TabButton` instances (e.g., 3), it's acceptable to define them individually without extracting them into a configuration to reduce repetition.
🧬 Code Definitions (2)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/policies/page.tsx (2)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/_policies/page.tsx (1) (1)
PoliciesPage
(3-13)apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/policies/PoliciesPageContent.tsx (1) (1)
PoliciesPageContent
(44-522)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/policies/PoliciesPageContent.tsx (2)
apps/webservice/src/app/urls.ts (1) (1)
urls
(197-197)apps/webservice/src/app/[workspaceSlug]/(app)/_components/policy/drawer/EnvironmentPolicyDrawer.tsx (2) (2)
param
(66-66)useEnvironmentPolicyDrawer
(67-98)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Typecheck
- GitHub Check: build (linux/amd64)
- GitHub Check: Lint
🔇 Additional comments (4)
apps/webservice/src/app/[workspaceSlug]/(app)/_components/environment/drawer/EnvironmentDrawer.tsx (1)
227-234
:❓ Verification inconclusive
Double-check override UI condition
Here, the
<UpdateOverridePolicy>
component is conditionally rendered usingenvironment.policy.isDefaultPolicy
. If the component is meant to configure overrides, ensure that showing it only for the "defaultPolicy" state is correct and aligns with the intended logic.Please verify that the logic correctly matches the new policy model (i.e., that "defaultPolicy" indeed controls when the override UX is displayed) by searching the codebase for references to
UpdateOverridePolicy
:
🏁 Script executed:
#!/bin/bash rg "UpdateOverridePolicy" -A 5Length of output: 5911
Double-check override UI condition – Verify Policy Trigger
The
<UpdateOverridePolicy>
component is rendered only whenenvironment.policy.isDefaultPolicy
is true. Please confirm that this condition aligns with the new policy model: specifically, that the override UI should only appear when the environment’s policy is in its default state. Ensure that this logic is consistent with the intended behavior of configuring policy overrides.
- Confirm that the default policy state is the correct trigger for displaying override options.
- Verify that any related changes in the new policy model don’t require additional conditions.
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/policies/page.tsx (1)
1-12
: Looks good and more explicitSwitching from a redirect-based approach to a
notFound()
response if the environment does not exist is a more explicit and user-friendly pattern. This ensures clarity about valid and invalid resources.apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/policies/PoliciesPageContent.tsx (2)
483-500
: Logic mismatch for "Release Interval"This logic still displays
environmentPolicy.releaseWindows.length
under "Release Interval," which may be misleading if the intent is to show an interval of time rather than just how many release windows exist. This comment was also raised in a past review.
66-69
:❓ Verification inconclusive
Verify navigation logic for environment policy editing
Here, depending on
hasParentPolicy
, you either push a new URL or set the localenvironmentPolicyId
. Confirm that this fully covers expected scenarios (e.g., does the system handle cases whenenvironmentPolicy.id
is invalid or missing?). Also ensure that end-users are routed to the correct policy editing experience.Run the following to check references of
onConfigurePolicyClick
and see how it’s used throughout:
🏁 Script executed:
#!/bin/bash rg "onConfigurePolicyClick" -A 5Length of output: 6871
Double-check navigation behavior for policy editing flows
The function currently handles navigation by:
- Pushing an external URL with a tab parameter when
hasParentPolicy
is true.- Invoking
setEnvironmentPolicyId(environmentPolicy.id, tab)
whenhasParentPolicy
is false.Please verify the following:
- Validation of
environmentPolicy.id
: Ensure that when there is no parent policy,environmentPolicy.id
is always valid. If this ID might be missing or invalid, consider adding proper error handling or a fallback mechanism.- Consistent navigation: Confirm that appending the tab value to
externalPolicyUrl
correctly routes users to the designated policy editing experience across all UI triggers (e.g., Approval, Concurrency, Management, DeploymentVersionChannels, Rollout).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (6)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/overview/_components/DeploymentTelemetryTable.tsx (6)
41-47
: Consider using a more semantic key for list itemsUsing an array index as a key can cause issues with component identity when the list changes. Consider using a more stable identifier if available:
{Object.values(versionDistro).map(({ percentage }, index) => ( <div - key={index} + key={`version-distro-${index}`} className={`h-full ${colors[index % colors.length]}`} style={{ width: `${percentage * 100}%` }} /> ))}
71-72
: Improve status indicator consistencyThe green dot indicator is hardcoded, but it should likely reflect the deployment's actual status for consistency with the status shown in the deployment status column.
<div className="flex items-center gap-2"> - <div className="h-2 w-2 rounded-full bg-green-500"></div> + <div className={cn("h-2 w-2 rounded-full", { + "bg-green-500": telemetry?.desiredVersion?.status === "Deployed", + "bg-blue-500": telemetry?.desiredVersion?.status === "Deploying", + "bg-red-500": telemetry?.desiredVersion?.status === "Failed", + "bg-amber-500": telemetry?.desiredVersion?.status === "Pending Approval", + "bg-neutral-500": !telemetry?.desiredVersion?.status + })}></div> <span className="text-sm text-neutral-200">{deployment.name}</span>
114-131
: Refactor duplicated status styling logicThe status styling logic is duplicated between the background and the dot indicator. Consider extracting this into a utility function or variable to avoid duplication:
+ const getStatusStyles = (status?: string) => ({ + background: { + "bg-green-500/10 text-green-400": status === "Deployed", + "bg-blue-500/10 text-blue-400": status === "Deploying", + "bg-red-500/10 text-red-400": status === "Failed", + "bg-amber-500/10 text-amber-400": status === "Pending Approval", + }, + dot: { + "bg-green-500": status === "Deployed", + "bg-blue-500": status === "Deploying", + "bg-red-500": status === "Failed", + "bg-amber-500": status === "Pending Approval", + } + }); + + const statusStyles = getStatusStyles(desiredVersion?.status); // Then use it in the JSX <span className={cn( "inline-flex items-center gap-1.5 rounded px-2 py-1 text-xs font-medium", - { - "bg-green-500/10 text-green-400": - desiredVersion.status === "Deployed", - "bg-blue-500/10 text-blue-400": - desiredVersion.status === "Deploying", - "bg-red-500/10 text-red-400": - desiredVersion.status === "Failed", - "bg-amber-500/10 text-amber-400": - desiredVersion.status === "Pending Approval", - }, + statusStyles.background, )} > <div className={cn("h-1.5 w-1.5 rounded-full", - { - "bg-green-500": desiredVersion.status === "Deployed", - "bg-blue-500": desiredVersion.status === "Deploying", - "bg-red-500": desiredVersion.status === "Failed", - "bg-amber-500": desiredVersion.status === "Pending Approval", - } + statusStyles.dot )} />
146-175
: Add accessibility attributes to the tableImprove accessibility by adding appropriate ARIA attributes to the table:
<h4 className="mb-3 text-sm font-medium text-neutral-300"> Deployment Versions </h4> <div className="overflow-hidden rounded-lg border border-neutral-800/50"> - <Table> + <Table aria-label="Deployment telemetry information"> <TableHeader> <TableRow className="border-b border-neutral-800/50 hover:bg-transparent"> <TableHead className="w-[200px] py-3 font-medium text-neutral-400"> Deployments </TableHead> {/* ... other headers ... */} </TableRow> </TableHeader> <TableBody> {deployments.length === 0 ? ( + <TableRow> + <TableCell colSpan={4} className="py-4 text-center text-neutral-400"> + No deployments available + </TableCell> + </TableRow> + ) : ( deployments.map((deployment) => ( <DeploymentRow key={deployment.id} deployment={deployment} /> )) + )} </TableBody> </Table> </div>
89-99
: Improve version distribution layoutThe current layout might have issues displaying multiple versions properly. Consider adding a separator and ensuring proper spacing:
{Object.entries(versionDistro).map(([version, { percentage }], index) => { return ( <div key={version} - className="flex items-center gap-1" + className="flex items-center gap-1 mr-2" style={{ width: `${percentage * 100}%` }} > - {version} + <span className="truncate">{version}</span> + <span className="whitespace-nowrap">{(percentage * 100).toFixed(1)}%</span> + {index < Object.entries(versionDistro).length - 1 && <span className="mx-1 text-neutral-600">|</span>} </div> ); })}
59-60
:❓ Verification inconclusive
Consider optimizing data fetching
The current approach fetches data for each deployment row separately. For better performance with many deployments, consider fetching all telemetry data at once in the parent component and passing it down:
// In DeploymentTelemetryTable + const { environmentId } = useParams<{ environmentId: string }>(); + const { data: telemetryMap, isLoading: telemetryLoading, error: telemetryError } = + api.environment.page.overview.telemetry.byDeployments.useQuery( + { environmentId, deploymentIds: deployments.map(d => d.id) }, + { refetchInterval: 60_000 }, + ); // Then pass to DeploymentRow <DeploymentRow key={deployment.id} deployment={deployment} + telemetry={telemetryMap?.[deployment.id]} + isLoading={telemetryLoading} + hasError={!!telemetryError} />
🏁 Script executed:
#!/bin/bash # Check if the required API endpoint exists rg "byDeployments" -A 5 -B 5Length of output: 28
Optimize Telemetry Data Fetching – Verify API Endpoint Availability
It appears that the current implementation triggers individual data fetches for each deployment row. To improve performance—especially when handling many deployments—consider consolidating telemetry data fetching at the parent component level and then passing the relevant data down to each
DeploymentRow
.Before proceeding with this refactoring, please ensure that the expected API endpoint exists and functions as required. In particular, verify that the call to:
api.environment.page.overview.telemetry.byDeployments.useQueryis available. If confirmed, you can refactor as follows:
// In DeploymentTelemetryTable.tsx + const { environmentId } = useParams<{ environmentId: string }>(); + const { + data: telemetryMap, + isLoading: telemetryLoading, + error: telemetryError, + } = api.environment.page.overview.telemetry.byDeployments.useQuery( + { environmentId, deploymentIds: deployments.map(d => d.id) }, + { refetchInterval: 60_000 }, + ); <DeploymentRow key={deployment.id} deployment={deployment} + telemetry={telemetryMap?.[deployment.id]} + isLoading={telemetryLoading} + hasError={!!telemetryError} />Action Items:
- Verify API Endpoint: Confirm that the endpoint
api.environment.page.overview.telemetry.byDeployments.useQuery
exists and is implemented as expected.- Optimize Data Fetching: If the endpoint is available, proceed with the refactoring to fetch all telemetry data once in the parent.
Please manually verify the existence and behavior of this API endpoint before moving forward.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/overview/_components/DeploymentTelemetryTable.tsx
(1 hunks)packages/api/src/router/environment-page/overview/desired-version.ts
(1 hunks)packages/api/src/router/environment-page/overview/router.ts
(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{ts,tsx}`: **Note on Error Handling:** Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error...
**/*.{ts,tsx}
: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
packages/api/src/router/environment-page/overview/desired-version.ts
packages/api/src/router/environment-page/overview/router.ts
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/overview/_components/DeploymentTelemetryTable.tsx
🧬 Code Definitions (1)
packages/api/src/router/environment-page/overview/router.ts (3)
packages/api/src/router/environment-page/overview/deployment-stats.ts (1) (1)
getDeploymentStats
(36-117)packages/api/src/router/environment-page/overview/version-distro.ts (1) (1)
getVersionDistro
(16-67)packages/api/src/router/environment-page/overview/desired-version.ts (1) (1)
getDesiredVersion
(7-104)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Typecheck
- GitHub Check: Lint
- GitHub Check: build (linux/amd64)
🔇 Additional comments (9)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/overview/_components/DeploymentTelemetryTable.tsx (1)
56-60
: Add error handling for API queryThe current implementation doesn't handle API errors. Consider adding error handling to improve user experience.
- const { data: telemetry, isLoading } = + const { data: telemetry, isLoading, error } = api.environment.page.overview.telemetry.byDeploymentId.useQuery( { environmentId, deploymentId: deployment.id }, { refetchInterval: 60_000 }, ); + // Add after line 61 + const hasError = !!error;Then update the UI to show an error state:
// At line 84-86 {isLoading ? "Loading distribution..." - : "No resources deployed"} + : hasError + ? "Failed to load data" + : "No resources deployed"}packages/api/src/router/environment-page/overview/desired-version.ts (3)
13-36
: Handle the null case for versionSelector.If
versionSelector
isnull
, thewhere
clause in the subsequent query might return unintended results or no rows. Verify that the upstream logic and callers can handle this scenario without errors.
53-55
: Validate null or undefined versionSelector in matching condition.
SCHEMA.deploymentVersionMatchesCondition(db, versionChannelSelector)
should gracefully handlenull
orundefined
to avoid query failures. Confirm that it checks for these cases before constructing SQL conditions.
88-101
: Confirm coverage for partial or mixed status scenarios.When some resources have not triggered any jobs, but others have, the logic defaults to "Deploying". Make sure the calling code interprets the "Deploying" status correctly in partial/mixed scenarios where some resources have no associated jobs.
packages/api/src/router/environment-page/overview/router.ts (5)
29-34
: Validate environment existence.Ensure the code checks for a non-existent environment return value to prevent runtime errors when accessing
environment.systemId
later.
40-52
: Good fallback for environments without filters.Returning default deployment statistics when
environment.resourceFilter
isnull
helps avoid errors. This approach keeps the code simple and handles edge cases gracefully.
58-63
: Efficiently exclude deleted resources.Using
isNull(SCHEMA.resource.deletedAt)
prevents pseudo-deleted resources from affecting the statistics. This helps maintain accuracy in reporting.
110-115
: Check for deployment existence.Validate that the fetched deployment is defined and handle the potential null return to avoid accessing
deployment.systemId
if undefined.
143-149
: Parallelize slow queries for performance.Running
getVersionDistro
andgetDesiredVersion
in parallel viaPromise.all
is a good approach. It reduces the overall response time.
{deployments.map((deployment) => ( | ||
<DeploymentRow key={deployment.id} deployment={deployment} /> | ||
))} | ||
</TableBody> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Add empty state handling for deployments
Add handling for when there are no deployments to display:
<TableBody>
+ {deployments.length === 0 ? (
+ <TableRow>
+ <TableCell colSpan={4} className="text-center py-6 text-neutral-400">
+ No deployments available
+ </TableCell>
+ </TableRow>
+ ) : (
{deployments.map((deployment) => (
<DeploymentRow key={deployment.id} deployment={deployment} />
))}
+ )}
</TableBody>
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
{deployments.map((deployment) => ( | |
<DeploymentRow key={deployment.id} deployment={deployment} /> | |
))} | |
</TableBody> | |
<TableBody> | |
{deployments.length === 0 ? ( | |
<TableRow> | |
<TableCell colSpan={4} className="text-center py-6 text-neutral-400"> | |
No deployments available | |
</TableCell> | |
</TableRow> | |
) : ( | |
deployments.map((deployment) => ( | |
<DeploymentRow key={deployment.id} deployment={deployment} /> | |
)) | |
)} | |
</TableBody> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/overview/_components/DeploymentTelemetryTable.tsx (2)
70-104
: Consider a configurable threshold and color mapping approach.Currently, the
DistroBar
lumps versions under 5% into the "other" segment. If 5% is a magic number or the color assignment for the "other" segment is always teal, you might consider making the threshold configurable and mapping each version (including "other") to a stable color. This helps ensure consistent behavior across deployments and re-renders.
158-158
: Consider dynamic coloring for the deployment indicator dot.A green dot is hard-coded to represent the deployment, which might be misleading if the actual deployment has a different status. If you intend to indicate the real-time status, consider matching it with the logic in the status column or removing the color if it’s purely decorative.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/overview/_components/DeploymentTelemetryTable.tsx
(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{ts,tsx}`: **Note on Error Handling:** Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error...
**/*.{ts,tsx}
: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/overview/_components/DeploymentTelemetryTable.tsx
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build (linux/amd64)
- GitHub Check: Typecheck
- GitHub Check: Lint
🔇 Additional comments (2)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/overview/_components/DeploymentTelemetryTable.tsx (2)
137-141
: Add error handling for the API query.This is a repeat of a past suggestion about handling API query errors here. The code currently ignores the
error
case from the TRPC query, which can lead to unhandled exceptions if the request fails. Consider adding error handling or a fallback UI for improved resiliency.
265-268
: Add an empty state row.This is a repeat of a past suggestion about handling the case when there are no deployments. Without an empty state row, the table appears blank or confusing if
deployments
is empty. Consider rendering a single row with a "No deployments available" message to improve clarity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 7
🧹 Nitpick comments (3)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/resources/ResourcesPageContent.tsx (3)
47-53
: Correct resource groupingThe
resourcesByVersion
grouping looks fine, butresourcesByKind
is actually grouping by both version and kind together, which might not be what was intended.const resourcesByVersion = _(resources) .groupBy((t) => t.version) .value() as Record<string, typeof resources>; - const resourcesByKind = _(resources) - .groupBy((t) => t.version + ": " + t.kind) - .value() as Record<string, typeof resources>; + const resourcesByKind = _(resources) + .groupBy((t) => t.kind) + .value() as Record<string, typeof resources>;
326-333
: Implement pagination functionalityThe pagination buttons are currently non-functional. Consider implementing actual pagination logic or removing them if not needed yet.
You could implement a simple pagination system by:
- Adding state for current page and items per page
- Slicing the resources array based on these values
- Enabling/disabling buttons based on page boundaries
1-569
: Consider breaking down this large componentThis component is very complex and handles multiple responsibilities including filtering, viewing, modal management, etc. Consider breaking it down into smaller, more focused components for better maintainability.
Potential components to extract:
- ResourceFilterModal
- ResourceStats
- ResourceList
- ResourceGrid
- ResourceFilterBar
🧰 Tools
🪛 Biome (1.9.4)
[error] 92-92: Comparing to itself is potentially pointless.
(lint/suspicious/noSelfCompare)
[error] 105-105: Comparing to itself is potentially pointless.
(lint/suspicious/noSelfCompare)
[error] 92-92: Unexpected constant condition.
(lint/correctness/noConstantCondition)
[error] 105-105: Unexpected constant condition.
(lint/correctness/noConstantCondition)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/layout.tsx
(4 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/resources/ResourceCard.tsx
(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/resources/ResourcesPageContent.tsx
(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/resources/page.tsx
(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/resources/useEnvResourceEditor.ts
(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/resources/useFilteredResources.ts
(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/layout.tsx
- apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/resources/page.tsx
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{ts,tsx}`: **Note on Error Handling:** Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error...
**/*.{ts,tsx}
: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/resources/useFilteredResources.ts
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/resources/useEnvResourceEditor.ts
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/resources/ResourcesPageContent.tsx
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/resources/ResourceCard.tsx
🪛 Biome (1.9.4)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/resources/ResourcesPageContent.tsx
[error] 92-92: Comparing to itself is potentially pointless.
(lint/suspicious/noSelfCompare)
[error] 105-105: Comparing to itself is potentially pointless.
(lint/suspicious/noSelfCompare)
[error] 92-92: Unexpected constant condition.
(lint/correctness/noConstantCondition)
[error] 105-105: Unexpected constant condition.
(lint/correctness/noConstantCondition)
🔇 Additional comments (4)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/resources/useFilteredResources.ts (1)
14-23
: Well-implemented hook for filtered resourcesThe
useFilteredResources
hook is cleanly implemented with proper TypeScript typing, documentation, and error handling. The pattern of returning both the raw query result and the extracted data with a fallback to an empty array is a good practice.apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/resources/useEnvResourceEditor.ts (2)
19-31
: LGTM - Helper function for processing resource filtersThe
getSelector
function properly handles different filter conditions and ensures a consistent structure for resource filters.
48-70
: Well-structured hook with proper mutation and cache invalidationThe implementation follows React hook patterns with good form state management and Promise chaining for updates. The cache invalidation after mutation ensures data consistency across the application.
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/resources/ResourceCard.tsx (1)
5-14
: Good status color mapping implementationThe status color mapping is well-defined with clear, semantic color choices for different resource states.
<div className="mt-3 space-y-2"> | ||
<div className="flex items-center justify-between text-xs"> | ||
<span className="text-neutral-400">Provider</span> | ||
<span className="text-neutral-300">{resource.providerId}</span> | ||
</div> | ||
|
||
<div className="flex items-center justify-between text-xs"> | ||
<span className="text-neutral-400">Deployment Success</span> | ||
<span | ||
// className={`text-${resource.healthScore > 90 ? "green" : resource.healthScore > 70 ? "amber" : "red"}-400`} | ||
> | ||
10% | ||
</span> | ||
</div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Remove duplicate information and implement dynamic deployment success
There are two issues in this section:
- The provider ID is displayed twice (also shown in the grid above)
- The deployment success percentage is hardcoded to "10%"
The commented-out line suggests there should be a dynamic value based on healthScore
.
<div className="mt-3 space-y-2">
<div className="flex items-center justify-between text-xs">
- <span className="text-neutral-400">Provider</span>
- <span className="text-neutral-300">{resource.providerId}</span>
+ <span className="text-neutral-400">Status</span>
+ <span className="text-neutral-300">{resourceStatus || "unknown"}</span>
</div>
<div className="flex items-center justify-between text-xs">
<span className="text-neutral-400">Deployment Success</span>
<span
- // className={`text-${resource.healthScore > 90 ? "green" : resource.healthScore > 70 ? "amber" : "red"}-400`}
+ className={`text-${resource.healthScore > 90 ? "green" : resource.healthScore > 70 ? "amber" : "red"}-400`}
>
- 10%
+ {resource.healthScore ?? 0}%
</span>
</div>
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
<div className="mt-3 space-y-2"> | |
<div className="flex items-center justify-between text-xs"> | |
<span className="text-neutral-400">Provider</span> | |
<span className="text-neutral-300">{resource.providerId}</span> | |
</div> | |
<div className="flex items-center justify-between text-xs"> | |
<span className="text-neutral-400">Deployment Success</span> | |
<span | |
// className={`text-${resource.healthScore > 90 ? "green" : resource.healthScore > 70 ? "amber" : "red"}-400`} | |
> | |
10% | |
</span> | |
</div> | |
<div className="mt-3 space-y-2"> | |
<div className="flex items-center justify-between text-xs"> | |
<span className="text-neutral-400">Status</span> | |
<span className="text-neutral-300">{resourceStatus || "unknown"}</span> | |
</div> | |
<div className="flex items-center justify-between text-xs"> | |
<span className="text-neutral-400">Deployment Success</span> | |
<span | |
className={`text-${resource.healthScore > 90 ? "green" : resource.healthScore > 70 ? "amber" : "red"}-400`} | |
> | |
{resource.healthScore ?? 0}% | |
</span> | |
</div> |
...)/systems/[systemSlug]/(raw)/environments/[environmentId]/resources/ResourcesPageContent.tsx
Outdated
Show resolved
Hide resolved
...)/systems/[systemSlug]/(raw)/environments/[environmentId]/resources/ResourcesPageContent.tsx
Outdated
Show resolved
Hide resolved
...)/systems/[systemSlug]/(raw)/environments/[environmentId]/resources/ResourcesPageContent.tsx
Outdated
Show resolved
Hide resolved
...)/systems/[systemSlug]/(raw)/environments/[environmentId]/resources/ResourcesPageContent.tsx
Outdated
Show resolved
Hide resolved
...)/systems/[systemSlug]/(raw)/environments/[environmentId]/resources/ResourcesPageContent.tsx
Outdated
Show resolved
Hide resolved
...)/systems/[systemSlug]/(raw)/environments/[environmentId]/resources/ResourcesPageContent.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 8
♻️ Duplicate comments (1)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/layout.tsx (1)
35-38
:⚠️ Potential issueHandle missing system scenario in layout.tsx
The API call fetches the system without any error handling. As demonstrated in related files and noted in a previous review, it's important to handle cases where the system data might not be found.
- const system = await api.system.bySlug({ - workspaceSlug, - systemSlug, - }); + const system = await api.system.bySlug({ + workspaceSlug, + systemSlug, + }).catch(() => notFound());This modification aligns the behavior with your existing environment check at line 33.
🧹 Nitpick comments (4)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/resources/ResourceCard.tsx (1)
80-94
: Provider information is duplicated and deployment success is hardcoded.There are two issues in this section:
- Provider information is duplicated (already displayed in the grid at line 70-71)
- Deployment success percentage is hardcoded to "10%" with a commented-out dynamic class
<div className="mt-3 space-y-2"> - <div className="flex items-center justify-between text-xs"> - <span className="text-neutral-400">Provider</span> - <span className="text-neutral-300">{resource.providerId}</span> - </div> <div className="flex items-center justify-between text-xs"> <span className="text-neutral-400">Deployment Success</span> <span - // className={`text-${resource.healthScore > 90 ? "green" : resource.healthScore > 70 ? "amber" : "red"}-400`} + className={`text-${resource.healthScore > 90 ? "green" : resource.healthScore > 70 ? "amber" : "red"}-400`} > - 10% + {resource.healthScore ?? 0}% </span> </div> </div>apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/resources/ResourcesPageContent.tsx (3)
476-498
: Use React refs instead of direct DOM manipulation.Direct DOM manipulation is an anti-pattern in React. Use React refs instead for accessing DOM elements.
+ const typeSelectRef = useRef<HTMLSelectElement>(null); + const valueSelectRef = useRef<HTMLSelectElement>(null); // Later in JSX: + <select ref={typeSelectRef} ...> + <select ref={valueSelectRef} ...> // And in the onClick function: onClick={() => { - const typeSelect = document.getElementById( - "condition-type", - ) as HTMLSelectElement; - const valueSelect = document.getElementById( - "condition-value", - ) as HTMLSelectElement; - if (typeSelect && valueSelect && valueSelect.value) { + if (typeSelectRef.current && valueSelectRef.current && valueSelectRef.current.value) { const newCondition = { - type: typeSelect.value, + type: typeSelectRef.current.value, operator: "equals", not: false, - value: valueSelect.value, + value: valueSelectRef.current.value, };
111-119
: Implement search functionality for the search input.The search input currently doesn't do anything. You should implement a search handler.
+ const [searchTerm, setSearchTerm] = useState(""); // Later in JSX: <Input placeholder="Search resources..." className="w-full pl-8 md:w-80" + value={searchTerm} + onChange={(e) => setSearchTerm(e.target.value)} /> // And then in the filteredResources assignment: - const filteredResources = resources; + const filteredResources = resources.filter(resource => + searchTerm === "" || + resource.name.toLowerCase().includes(searchTerm.toLowerCase()) || + resource.identifier.toLowerCase().includes(searchTerm.toLowerCase()) + );
205-208
: Add loading and error states.Resources might be in a loading or error state, which isn't handled currently. It's a good practice to provide feedback to users.
{selectedView === "grid" ? ( <div className="grid grid-cols-1 gap-4 sm:grid-cols-2 lg:grid-cols-3 xl:grid-cols-4"> + {resources.length === 0 && ( + <div className="col-span-full text-center py-8 text-neutral-400"> + No resources found + </div> + )} {filteredResources.map((resource) => ( <ResourceCard key={resource.id} resource={resource} /> ))} </div>
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/layout.tsx
(4 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/resources/ResourceCard.tsx
(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/resources/ResourcesPageContent.tsx
(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/resources/page.tsx
(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/resources/useEnvResourceEditor.ts
(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/resources/useFilteredResources.ts
(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/resources/page.tsx
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{ts,tsx}`: **Note on Error Handling:** Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error...
**/*.{ts,tsx}
: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/resources/useFilteredResources.ts
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/resources/ResourceCard.tsx
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/resources/useEnvResourceEditor.ts
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/layout.tsx
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/resources/ResourcesPageContent.tsx
🧬 Code Definitions (2)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/layout.tsx (2)
apps/webservice/src/app/urls.ts (1) (1)
urls
(197-197)apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/_components/EnvironmentTabs.tsx (1) (1)
EnvironmentTabs
(11-59)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/resources/ResourcesPageContent.tsx (2)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/resources/useFilteredResources.ts (1) (1)
useFilteredResources
(14-23)apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/resources/ResourceCard.tsx (1) (1)
ResourceCard
(17-97)
🪛 Biome (1.9.4)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/resources/ResourcesPageContent.tsx
[error] 92-92: Comparing to itself is potentially pointless.
(lint/suspicious/noSelfCompare)
[error] 105-105: Comparing to itself is potentially pointless.
(lint/suspicious/noSelfCompare)
[error] 92-92: Unexpected constant condition.
(lint/correctness/noConstantCondition)
[error] 105-105: Unexpected constant condition.
(lint/correctness/noConstantCondition)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Typecheck
- GitHub Check: Lint
- GitHub Check: build (linux/amd64)
🔇 Additional comments (6)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/layout.tsx (4)
1-1
: Import statements updated to support layout changesThe imports have been updated to remove sidebar-related components and add the new
EnvironmentTabs
component. This aligns with the simplified layout approach in the refactored environment page.Also applies to: 15-15, 21-21
43-44
: Layout structure simplifiedThe layout has been improved by replacing the previous
SidebarProvider
wrapper with a standarddiv
, and updating the navigation links. This simplification makes the code more maintainable.Also applies to: 46-46
52-57
: Enhanced breadcrumb navigationAdding the system name to the breadcrumb navigation improves user context and makes the navigation hierarchy clearer, helping users understand where they are in the application structure.
76-89
: Improved content structure with tabsThe new layout provides a better structure with:
- A clear title and description section
- The new
EnvironmentTabs
component for improved navigation- A cleaner container for the children components
This improves both the visual hierarchy and user experience of the environment page.
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/resources/useFilteredResources.ts (1)
1-23
: Custom hook implementation looks good!The
useFilteredResources
hook is well-structured with proper typing, error handling, and query enablement conditions. The JSDoc documentation clearly explains the purpose and return values.apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/resources/useEnvResourceEditor.ts (1)
33-70
: Well-implemented resource editor hook.The hook combines form state management, API interactions, and proper chaining of promises for mutations and query invalidation. The schema definition and utility functions are clear and appropriately separated.
...)/systems/[systemSlug]/(raw)/environments/[environmentId]/resources/ResourcesPageContent.tsx
Outdated
Show resolved
Hide resolved
...)/systems/[systemSlug]/(raw)/environments/[environmentId]/resources/ResourcesPageContent.tsx
Outdated
Show resolved
Hide resolved
...)/systems/[systemSlug]/(raw)/environments/[environmentId]/resources/ResourcesPageContent.tsx
Outdated
Show resolved
Hide resolved
...)/systems/[systemSlug]/(raw)/environments/[environmentId]/resources/ResourcesPageContent.tsx
Outdated
Show resolved
Hide resolved
...)/systems/[systemSlug]/(raw)/environments/[environmentId]/resources/ResourcesPageContent.tsx
Outdated
Show resolved
Hide resolved
...)/systems/[systemSlug]/(raw)/environments/[environmentId]/resources/ResourcesPageContent.tsx
Outdated
Show resolved
Hide resolved
...)/systems/[systemSlug]/(raw)/environments/[environmentId]/resources/ResourcesPageContent.tsx
Outdated
Show resolved
Hide resolved
...)/systems/[systemSlug]/(raw)/environments/[environmentId]/resources/ResourcesPageContent.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🧹 Nitpick comments (15)
packages/validators/src/jobs/index.ts (1)
53-61
: Consider adding JSDoc comments for the new constants.While the constant names are descriptive, adding JSDoc comments would improve documentation and provide additional context for developers who might use these constants.
// Statuses that should be included in the analytics calculations export const analyticsStatuses = [ JobStatus.Failure, JobStatus.InvalidJobAgent, JobStatus.InvalidIntegration, JobStatus.ExternalRunNotFound, JobStatus.Successful, ]; +/** + * Statuses that indicate a job has failed for any reason + */ export const failedStatuses = [ JobStatus.Failure, JobStatus.InvalidJobAgent, JobStatus.InvalidIntegration, JobStatus.ExternalRunNotFound, ]; +/** + * Statuses that indicate a job was not deployed because it was deliberately skipped or cancelled + */ export const notDeployedStatuses = [JobStatus.Skipped, JobStatus.Cancelled];apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/resources/_hooks/useFilteredResources.ts (1)
7-13
: Fix parameter name in JSDoc comment.The JSDoc comment mentions
workspaceId
as a parameter, but the function acceptsenvironmentId
instead./** * Hook for fetching resources based on a filter condition * - * @param workspaceId - ID of the workspace to fetch resources from + * @param environmentId - ID of the environment to fetch resources from * @param filter - Optional resource filter condition + * @param limit - Optional limit for the number of resources to fetch + * @param offset - Optional offset for pagination * @returns Query result containing filtered resources */apps/webservice/src/app/[workspaceSlug]/(app)/_components/resources/condition/ResourceVersionConditionRender.tsx (2)
8-40
: Add error handling for API queriesThe component correctly handles loading states but doesn't account for error states from the API queries. If either the workspace or versions query fails, this isn't reflected in the UI.
Consider adding error handling and a fallback UI for error states:
- const loading = workspace.isLoading || versions.isLoading; + const loading = workspace.isLoading || versions.isLoading; + const error = workspace.isError || versions.isError; + + if (error) { + return ( + <div className={className}> + <p className="text-red-500">Failed to load versions. Please try again.</p> + </div> + ); + }
22-26
: Add fallback for empty versions arrayThe component maps over versions data but doesn't handle the case when no versions are available after successful loading.
Consider adding a check for empty versions and providing a user-friendly message:
+ const noVersionsAvailable = versions.isSuccess && (versions.data?.length ?? 0) === 0; + const options = (versions.data ?? []).map((version) => ({ key: version, value: version, display: version, })); const loading = workspace.isLoading || versions.isLoading; return ( + <> + {noVersionsAvailable && ( + <div className="text-sm text-neutral-400 mb-2"> + No versions available for selection + </div> + )} <ChoiceConditionRender type="version" onSelect={setVersion} selected={selectedVersion ?? null} options={options} className={className} loading={loading} /> + </> );packages/api/src/router/environment-page/resources/router.ts (3)
46-52
: Merging environment and user-provided filters into a single comparison object is clean.Ensure
environment.resourceFilter
won’t be null or invalid. If so, you may want to gracefully handle it or provide a fallback to avoid unexpected errors.
54-75
: Efficient resource selection with limit/offset.This approach is good for medium-sized datasets. For potentially large data, consider more advanced pagination (e.g., keyset pagination) to mitigate performance risks.
105-139
: Health computation logic covers known statuses.Updates to job statuses (e.g., introducing new failure states) might require revision to the analytics/failed status arrays. Keep these enumerations in sync with any future status additions.
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/resources/ResourcesPageContent.tsx (3)
58-68
: Minimal but effective pagination approach.Storing the page in the URL is shareable and straightforward. If the dataset grows large, consider an alternative strategy to reduce offset-based queries.
172-309
: Comprehensive resource summary.Good breakdown of healthy, unhealthy, deploying counts. Each filter call is O(n); if the resource list becomes large, caching or memoization might help.
312-415
: Search and filter UI is user-friendly.Debounced input reduces unnecessary calls. Though concurrency issues don’t appear likely, be mindful if future expansions add multiple parallel queries or advanced interactions.
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/resources/_components/ResourceCard.tsx (5)
45-51
: Add error handling for clipboard operations.The clipboard API should include error handling as it may fail in certain browsers or contexts.
const handleCopyId = () => { - navigator.clipboard.writeText(resource.id); - toast("Resource ID copied", { - description: resource.id, - duration: 2000, - }); + navigator.clipboard.writeText(resource.id) + .then(() => { + toast("Resource ID copied", { + description: resource.id, + duration: 2000, + }); + }) + .catch((error) => { + toast("Failed to copy ID", { + description: "Please try again or copy manually", + variant: "destructive", + duration: 3000, + }); + console.error("Failed to copy to clipboard:", error); + }); };
75-86
: Improve ID truncation display.The current ID truncation approach (
split("-").at(0)
) is somewhat brittle. Consider using a more robust approach that ensures consistent display regardless of ID format.<div className="flex items-center justify-end gap-1"> <span className="truncate text-neutral-300"> - {resource.id.split("-").at(0)}... + {resource.id.substring(0, 8)}... </span> <button onClick={handleCopyId} className="text-muted-foreground hover:text-neutral-200" title="Copy ID" > <IconCopy size={14} /> </button> </div>
94-95
: Add fallback text for missing provider.When provider is null, an empty string is displayed. Consider adding a meaningful fallback text.
<div className="truncate text-muted-foreground">Provider</div> -<PropertyWithTooltip content={resource.provider?.name ?? ""} /> +<PropertyWithTooltip content={resource.provider?.name ?? "No provider"} />
97-101
: Improve date formatting with error handling.The date formatting could fail if the date values are invalid. Consider adding error handling and a more consistent date format.
<div className="truncate text-muted-foreground">Updated</div> <div className="truncate text-right text-neutral-300"> - {resource.updatedAt?.toLocaleDateString() ?? - resource.createdAt.toLocaleDateString()} + {(() => { + try { + return resource.updatedAt?.toLocaleDateString() ?? + resource.createdAt.toLocaleDateString(); + } catch (e) { + return "Invalid date"; + } + })()} </div>
104-106
: Enhance percentage display formatting.Consider adding a more robust formatting for the percentage to handle edge cases like NaN or negative values.
<div className="truncate text-muted-foreground">Deployment Success</div> <div className="truncate text-right text-neutral-300"> - {(resource.successRate * 100).toFixed(0)}% + {(() => { + const rate = resource.successRate || 0; + return `${Math.max(0, Math.min(100, rate * 100)).toFixed(0)}%`; + })()} </div>
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/resources/ResourcesPageContent.tsx
(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/resources/_components/ResourceCard.tsx
(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/resources/_components/ResourceTable.tsx
(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/resources/_hooks/useEnvResourceEditor.ts
(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/resources/_hooks/useFilteredResources.ts
(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/resources/page.tsx
(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/_components/resources/condition/ComparisonConditionRender.tsx
(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/_components/resources/condition/ResourceConditionBadge.tsx
(4 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/_components/resources/condition/ResourceConditionRender.tsx
(3 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/_components/resources/condition/ResourceVersionConditionRender.tsx
(1 hunks)packages/api/src/router/environment-page/resources/router.ts
(1 hunks)packages/api/src/router/environment-page/router.ts
(1 hunks)packages/api/src/router/resources.ts
(1 hunks)packages/db/src/schema/resource.ts
(1 hunks)packages/validators/src/jobs/index.ts
(1 hunks)packages/validators/src/resources/conditions/comparison-condition.ts
(4 hunks)packages/validators/src/resources/conditions/index.ts
(1 hunks)packages/validators/src/resources/conditions/resource-condition.ts
(6 hunks)packages/validators/src/resources/conditions/version-condition.ts
(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/api/src/router/environment-page/router.ts
- apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/resources/page.tsx
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{ts,tsx}`: **Note on Error Handling:** Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error...
**/*.{ts,tsx}
: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
packages/validators/src/resources/conditions/index.ts
packages/validators/src/resources/conditions/comparison-condition.ts
apps/webservice/src/app/[workspaceSlug]/(app)/_components/resources/condition/ComparisonConditionRender.tsx
packages/db/src/schema/resource.ts
packages/validators/src/resources/conditions/version-condition.ts
packages/validators/src/jobs/index.ts
packages/api/src/router/resources.ts
packages/validators/src/resources/conditions/resource-condition.ts
apps/webservice/src/app/[workspaceSlug]/(app)/_components/resources/condition/ResourceConditionRender.tsx
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/resources/_hooks/useFilteredResources.ts
apps/webservice/src/app/[workspaceSlug]/(app)/_components/resources/condition/ResourceConditionBadge.tsx
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/resources/_components/ResourceTable.tsx
apps/webservice/src/app/[workspaceSlug]/(app)/_components/resources/condition/ResourceVersionConditionRender.tsx
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/resources/ResourcesPageContent.tsx
packages/api/src/router/environment-page/resources/router.ts
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/resources/_hooks/useEnvResourceEditor.ts
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/resources/_components/ResourceCard.tsx
🧬 Code Definitions (9)
packages/validators/src/resources/conditions/comparison-condition.ts (1)
packages/validators/src/resources/conditions/version-condition.ts (2) (2)
versionCondition
(3-7)VersionCondition
(9-9)
packages/validators/src/jobs/index.ts (1)
packages/db/src/schema/job.ts (1) (1)
JobStatus
(132-132)
apps/webservice/src/app/[workspaceSlug]/(app)/_components/resources/condition/ResourceConditionRender.tsx (2)
packages/validators/src/resources/conditions/resource-condition.ts (1) (1)
isVersionCondition
(134-137)apps/webservice/src/app/[workspaceSlug]/(app)/_components/resources/condition/ResourceVersionConditionRender.tsx (1) (1)
ResourceVersionConditionRender
(8-40)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/resources/_hooks/useFilteredResources.ts (1)
packages/validators/src/resources/conditions/resource-condition.ts (1) (1)
ResourceCondition
(27-36)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/resources/_components/ResourceTable.tsx (1)
packages/db/src/schema/resource.ts (2) (2)
Resource
(103-103)resource
(58-86)
apps/webservice/src/app/[workspaceSlug]/(app)/_components/resources/condition/ResourceVersionConditionRender.tsx (1)
packages/validators/src/resources/conditions/version-condition.ts (1) (1)
VersionCondition
(9-9)
packages/api/src/router/environment-page/resources/router.ts (2)
packages/validators/src/resources/conditions/resource-condition.ts (2) (2)
resourceCondition
(38-48)ResourceCondition
(27-36)packages/validators/src/jobs/index.ts (3) (3)
analyticsStatuses
(46-52)failedStatuses
(54-59)activeStatus
(32-32)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/resources/_hooks/useEnvResourceEditor.ts (1)
packages/validators/src/resources/conditions/resource-condition.ts (3) (3)
ResourceCondition
(27-36)isComparisonCondition
(76-79)resourceCondition
(38-48)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/resources/_components/ResourceCard.tsx (1)
packages/db/src/schema/resource.ts (2) (2)
Resource
(103-103)resource
(58-86)
🪛 Biome (1.9.4)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/resources/ResourcesPageContent.tsx
[error] 432-432: Comparing to itself is potentially pointless.
(lint/suspicious/noSelfCompare)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Lint
- GitHub Check: Typecheck
- GitHub Check: build (linux/amd64)
🔇 Additional comments (33)
packages/validators/src/jobs/index.ts (2)
54-59
: Clear grouping of failed job statuses.The new
failedStatuses
array provides a useful grouping of all job statuses that indicate failure. This will help standardize failure detection across the application and improve code reusability.
61-61
: Logical grouping of non-deployed statuses.The
notDeployedStatuses
array appropriately groups statuses where a job wasn't deployed due to being deliberately skipped or cancelled. This distinction from failed statuses is important for accurate reporting and UI displays.packages/validators/src/resources/conditions/index.ts (1)
8-8
: LGTM! Added version condition export.The export of version-condition.js aligns with the existing pattern for other condition types, maintaining consistency in how conditions are exposed from this module.
apps/webservice/src/app/[workspaceSlug]/(app)/_components/resources/condition/ComparisonConditionRender.tsx (1)
346-356
: LGTM! Added Version condition to dropdown menu.This addition correctly exposes the newly created Version condition type to users in the dropdown menu, following the same pattern as other condition types with appropriate default values.
packages/validators/src/resources/conditions/version-condition.ts (1)
1-9
: LGTM! Well-structured version condition implementation.The
versionCondition
schema is properly defined using Zod with appropriate validation constraints, and the TypeScript type is correctly exported using type inference from the schema. This follows the established pattern for other condition types in the codebase.packages/validators/src/resources/conditions/comparison-condition.ts (4)
12-12
: LGTM! Type import for VersionCondition.Correctly imported the VersionCondition type following the pattern established for other condition types.
22-22
: LGTM! Schema import for versionCondition.Properly imported the versionCondition schema consistent with other condition imports.
39-39
: LGTM! Added version condition to schema.Successfully integrated versionCondition into the comparisonCondition schema's union of condition types.
58-58
: LGTM! Added VersionCondition to type union.Properly updated the ComparisonCondition type to include VersionCondition in the conditions array type union.
packages/db/src/schema/resource.ts (1)
305-306
: LGTM! Version comparison added correctly.The added condition for checking
ResourceFilterType.Version
follows the established pattern and integrates well with the existing filter system. This will enable filtering resources by their version values.packages/api/src/router/resources.ts (1)
726-740
: LGTM! Well-implemented version query endpoint.The new
versions
procedure follows best practices:
- Proper input validation using zod
- Authentication & authorization checks
- Efficient database query using selectDistinct
- Properly filters out deleted resources
- Clean result transformation
This will provide a reliable way to retrieve all distinct versions for a workspace.
apps/webservice/src/app/[workspaceSlug]/(app)/_components/resources/condition/ResourceConditionRender.tsx (3)
13-13
: LGTM! Clean import addition.Added import for the
isVersionCondition
type guard that will be used to determine when to render the version condition component.
25-25
: LGTM! Import for version condition renderer.Adding the import for the
ResourceVersionConditionRender
component that will handle rendering of version conditions.
107-114
: LGTM! Version condition rendering is well-implemented.The condition check and rendering logic follows the established pattern in this file. The component passes all the necessary props to the
ResourceVersionConditionRender
component.apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/resources/_hooks/useFilteredResources.ts (1)
14-25
: LGTM! Well-implemented hook for resource filtering.This hook is cleanly implemented and:
- Properly handles optional parameters
- Uses nullish coalescing to handle undefined filters
- Prevents unnecessary queries when environmentId is empty
- Provides a default empty array for resources when data is null/undefined
This will make resource filtering more reusable across components.
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/resources/_hooks/useEnvResourceEditor.ts (2)
19-31
: Well-structured helper function for selector transformationThe
getSelector
function effectively handles the transformation of resource filters, ensuring that single conditions are properly wrapped in a comparison condition when needed. This is a good abstraction that simplifies the form handling logic.
48-70
: Clean and well-documented hook implementationThe hook follows React best practices with proper mutation handling, form state management, and query invalidation. The Promise chain for handling the form submission is clean and ensures that the UI state is properly updated after successful mutations.
packages/validators/src/resources/conditions/resource-condition.ts (2)
13-13
: Properly integrated VersionCondition supportThe new version condition type is correctly integrated into the existing condition system, following the established patterns for imports, type definitions, and schema validation.
Also applies to: 25-25, 36-36, 47-47, 66-66
134-137
: Well-implemented type guard functionThe
isVersionCondition
type guard follows the same pattern as other type guards in the file, maintaining consistency in the codebase.apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/resources/_components/ResourceTable.tsx (3)
23-27
: Good type extension approachExtending the
SCHEMA.Resource
type with additional properties needed for the UI is a clean approach that maintains type safety while accommodating the component's rendering requirements.
29-77
: Well-structured resource row with appropriate conditionalsThe
ResourceRow
component effectively displays resource information with appropriate styling and conditional rendering. The success rate visualization with the colored progress bar is particularly well-implemented.
79-119
: Responsive table layout with clear column structureThe
ResourceTable
component has a well-defined structure with appropriate column widths and clear headers. The table is wrapped in a container with proper styling for overflow and borders.apps/webservice/src/app/[workspaceSlug]/(app)/_components/resources/condition/ResourceConditionBadge.tsx (4)
13-13
: No issues with the new VersionCondition import.It matches the introduced functionality for version-based conditions and seems well-aligned with the existing types.
36-36
: Good addition of 'isVersionCondition' helper.The new type guard broadens the scope of condition checks to handle version conditions properly.
251-262
: Implementation of StringifiedVersionCondition looks solid.This design is consistent with other condition visualizations, displaying the operator verb and version as expected.
304-305
: Seamless integration of version conditions.Using the new condition check ensures version conditions get the correct formatting component. No issues found.
packages/api/src/router/environment-page/resources/router.ts (3)
23-31
: The 'list' procedure's input schema is solid.The Zod definitions for environmentId, filter, limit, and offset are well-structured and ensure valid queries.
76-104
: Retrieval of job triggers and ordering logic.Selecting distinct triggers by resource and deployment ensures you only get the latest job. Validate that this meets the needs of multi-version concurrency and older job references.
Can you confirm whether older concurrent job runs could affect resource health classification?
140-145
: Result mapping with health status is clear and well-structured.Merging the computed health data into the resource objects makes for a concise, readable final output.
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/resources/ResourcesPageContent.tsx (2)
70-129
: Clear and modular filter setup.Splitting filter operations into parseResourceFilter, getResourceFilterFromDropdownChange, and getResourceFilterWithSearch helps keep code maintainable.
417-429
: Grid and list views both enhance user experience.Loading skeletons are a nice touch for perceived performance. Implementation looks solid.
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/resources/_components/ResourceCard.tsx (2)
13-19
: LGTM: Clear status color mapping.The status color mapping is well-defined and provides visual cues for resource health status.
21-34
: LGTM: Well-implemented reusable tooltip component.The
PropertyWithTooltip
component is a clean, reusable solution for displaying truncated text with tooltips.
<div className="flex items-center gap-2"> | ||
<div className="h-1.5 w-16 rounded-full bg-neutral-800"> | ||
<div | ||
style={{ width: `${resource.successRate * 100}%` }} | ||
className={cn( | ||
"h-full rounded-full", | ||
resource.successRate >= 0.9 | ||
? "bg-green-500" | ||
: resource.successRate >= 0.5 | ||
? "bg-yellow-500" | ||
: "bg-red-500", | ||
)} | ||
/> | ||
</div> | ||
<span className="text-sm"> | ||
{(resource.successRate * 100).toFixed(0)}% | ||
</span> | ||
</div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Verification agent
🧩 Analysis chain
Consider adding error handling for success rate display
The current implementation assumes the successRate
property will always be present and valid. Consider adding a fallback or validation to handle cases where this data might be missing or invalid.
🏁 Script executed:
#!/bin/bash
# Check if resources.successRate is used elsewhere with validation or preprocessing
rg -A 3 -B 3 "successRate" --type ts --type tsx
Length of output: 75
Action Required: Add Fallback for Missing/Invalid successRate
The component in apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/resources/_components/ResourceTable.tsx
(lines 49–66) currently assumes that resource.successRate
is always a valid numeric value. Our initial verification couldn't locate any centralized validation or fallback handling for this property.
Please consider adding error handling (or a default value) before performing arithmetic operations on resource.successRate
—for example:
- Update the width style:
style={{ width: `${(resource.successRate ?? 0) * 100}%` }}
- Adjust the display to safely compute the percentage:
{(resource.successRate ?? 0) * 100).toFixed(0)}%
- Ensure that the conditional classNames (green/yellow/red) handle cases where
resource.successRate
is missing or not a valid number.
If there is existing validation logic elsewhere that should cover this case, please confirm its application here; otherwise, the component would benefit from this added safeguard.
---
🏁 Script executed:
```shell
#!/bin/bash
# Re-run the search for "successRate" usage in both TypeScript and TSX files using glob patterns
rg -A 3 -B 3 "resource\.successRate" -g "*.tsx" -g "*.ts"
Length of output: 5120
Action Required: Add Fallback/Validation for resource.successRate
It appears that both the ResourceTable and ResourceCard components assume that resource.successRate
is always available and a valid number. Our search confirms this usage in both files without any error handling. To prevent potential runtime errors (or unexpected UI behavior) when the value is missing or invalid, please consider updating these components. For example:
-
In ResourceTable.tsx:
<div - style={{ width: `${resource.successRate * 100}%` }} + style={{ width: `${(resource.successRate ?? 0) * 100}%` }} - className={cn( - "h-full rounded-full", - resource.successRate >= 0.9 - ? "bg-green-500" - : resource.successRate >= 0.5 - ? "bg-yellow-500" - : "bg-red-500", - )} + className={cn( + "h-full rounded-full", + (resource.successRate ?? 0) >= 0.9 + ? "bg-green-500" + : (resource.successRate ?? 0) >= 0.5 + ? "bg-yellow-500" + : "bg-red-500", + )}
And update the percentage display accordingly:
- {(resource.successRate * 100).toFixed(0)}% + {((resource.successRate ?? 0) * 100).toFixed(0)}%
-
In ResourceCard.tsx:
Make similar updates whereverresource.successRate
is used to ensure safe fallback handling.
Review and test these changes to ensure that the UI gracefully handles any missing or invalid successRate
values.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
<div className="flex items-center gap-2"> | |
<div className="h-1.5 w-16 rounded-full bg-neutral-800"> | |
<div | |
style={{ width: `${resource.successRate * 100}%` }} | |
className={cn( | |
"h-full rounded-full", | |
resource.successRate >= 0.9 | |
? "bg-green-500" | |
: resource.successRate >= 0.5 | |
? "bg-yellow-500" | |
: "bg-red-500", | |
)} | |
/> | |
</div> | |
<span className="text-sm"> | |
{(resource.successRate * 100).toFixed(0)}% | |
</span> | |
</div> | |
<div className="flex items-center gap-2"> | |
<div className="h-1.5 w-16 rounded-full bg-neutral-800"> | |
<div | |
style={{ width: `${(resource.successRate ?? 0) * 100}%` }} | |
className={cn( | |
"h-full rounded-full", | |
(resource.successRate ?? 0) >= 0.9 | |
? "bg-green-500" | |
: (resource.successRate ?? 0) >= 0.5 | |
? "bg-yellow-500" | |
: "bg-red-500", | |
)} | |
/> | |
</div> | |
<span className="text-sm"> | |
{((resource.successRate ?? 0) * 100).toFixed(0)}% | |
</span> | |
</div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
successRate always defined
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@adityachoudhari26 Thank you for the clarification. Since you've confirmed that successRate
is always defined, the current implementation without fallback handling is correct. I'll make note of this for future reference.
✏️ Learnings added
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#395
File: apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/resources/_components/ResourceTable.tsx:49-66
Timestamp: 2025-03-24T18:52:39.569Z
Learning: The `successRate` property in resource components is always defined and does not require fallback handling or null checks.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
...)/systems/[systemSlug]/(raw)/environments/[environmentId]/resources/ResourcesPageContent.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
♻️ Duplicate comments (5)
packages/api/src/router/environment-page/overview/router.ts (3)
35-45
:⚠️ Potential issueAdd a check for non-existent environment before referencing
environment.systemId
.If the
environment
variable is null or undefined, referencingenvironment.systemId
orenvironment.resourceFilter
will throw a runtime error. You must ensure that the environment is defined before using its fields.const environment = await ctx.db .select() .from(SCHEMA.environment) .where(eq(SCHEMA.environment.id, environmentId)) .then(takeFirst); + if (!environment) { + throw new TRPCError({ + code: "NOT_FOUND", + message: `Environment ${environmentId} does not exist`, + }); + } const deployments = await ctx.db .select() .from(SCHEMA.deployment) - .where(eq(SCHEMA.deployment.systemId, environment.systemId)); + .where(eq(SCHEMA.deployment.systemId, environment?.systemId));
123-126
:⚠️ Potential issueAdd a check for non-existent deployment before referencing
deployment.resourceFilter
.If
deployment
is null or undefined, accessingdeployment.resourceFilter
will throw a runtime error.const [environment, deployment] = await Promise.all([ envPromise, deploymentPromise, ]); + if (!deployment) { + throw new TRPCError({ + code: "NOT_FOUND", + message: `Deployment ${deploymentId} does not exist`, + }); + }
127-128
:⚠️ Potential issueAlso check if
environment
is null before accessingenvironment.resourceFilter
.Line 128 attempts to access
environment.resourceFilter
, which will throw ifenvironment
is null or undefined.if (environment.resourceFilter == null) return undefined;packages/api/src/router/environment-page/deployments/router.ts (1)
232-236
:⚠️ Potential issueAdd a check for non-existent environment before referencing
environment.systemId
.If
environment
is null, referencingenvironment.systemId
(or other environment fields) will throw a runtime error. Incorporate a null check or throw an error if the environment cannot be found.const environment = await ctx.db .select() .from(SCHEMA.environment) .where(eq(SCHEMA.environment.id, environmentId)) .then(takeFirst); + if (!environment) { + throw new TRPCError({ + code: "NOT_FOUND", + message: `Environment ${environmentId} does not exist`, + }); + } const deployments = await ctx.db .select() .from(SCHEMA.deployment) - .where(eq(SCHEMA.deployment.systemId, environment.systemId)); + .where(eq(SCHEMA.deployment.systemId, environment?.systemId));Also applies to: 243-243
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/resources/ResourcesPageContent.tsx (1)
433-447
:⚠️ Potential issueFix the self-comparison condition.
There's a self-comparison that is always true:
resources.length === resources.length
. This should likely be comparing different values or checking a specific condition.-{resources.length === resources.length ? ( +{resources.length > 0 ? ( <>Showing all {resources.length} resources</> ) : ( <> Showing {resources.length} of {resources.length} resources </> )}🧰 Tools
🪛 Biome (1.9.4)
[error] 435-435: Comparing to itself is potentially pointless.
(lint/suspicious/noSelfCompare)
🧹 Nitpick comments (13)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/deployments/_components/StatusBadge.tsx (1)
3-58
: Consider centralizing style & text logic to reduce repetition.Each status variation is handled in a similar conditional pattern—extracting a mapping of status → style and text could improve maintainability. For instance, a single object keyed by status could define the badge class names and label text, selected by
statusLower
. This avoids multipleif
conditions and can make adding new statuses easier in the future.apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/deployments/EnvironmentDeploymentsPageContent.tsx (3)
57-121
: Remove or clarify large blocks of commented code.This chunk of commented-out “deployment summary cards” can confuse future maintainers. If it’s a work-in-progress, leave a clear TODO note describing when it will be activated. Otherwise, remove it to keep the codebase lean.
135-159
: Consider advanced filter handling or dedicated filter components.The filter options are defined inline with simple
<select>
elements. If you plan to add more complex filter logic or additional filter categories, consider extracting them into a dedicated component or using a more structured approach (e.g., a filter context or library) to enhance readability and maintainability.
196-249
: Provide user-friendly feedback for empty or errored states.If
deploymentStats
is empty or the request encounters an error, the UI provides no clear user feedback apart from an empty table. Consider adding an “No deployments found” message or an error alert to improve user experience and clarity.packages/api/src/router/environment-page/deployments/router.ts (2)
250-257
: Consider indexing thedeployment.name
column for improved search performance.If the dataset is large, searching deployments by name (
ilike
) could become slow. Creating a suitable index may help mitigate any performance constraints.
112-212
: Add a guard for null environment or deployment ingetDeploymentStats
.Although environment and deployment references are likely validated before calling
getDeploymentStats
, ensure that either the function or its caller gracefully handles the scenario where these objects might be undefined or null.const getDeploymentStats = async ( db: Tx, environment: SCHEMA.Environment | null, deployment: SCHEMA.Deployment | null, workspaceId: string, ) => { + if (!environment || !deployment) { + return null; + } ... };apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/resources/_hooks/useFilteredResources.ts (2)
14-20
: Consider providing defaults or validations for limit/offset
Allowing custom pagination parameters is great, but consider whether you want to enforce default values (e.g., default limit) or define upper bounds to prevent excessive requests.
25-25
: Return object shape
Returning{ ...resourcesQ, resources: resourcesQ.data ?? [] }
is a helpful pattern. Just be aware that ifresourcesQ
ever includes aresources
field itself, it could be overshadowed.apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/resources/page.tsx (1)
23-37
: Handling null resourceFilter
Displaying a descriptive message when no filter is set is user-friendly. As an enhancement, consider providing a button or link for users to define a resource filter here.packages/api/src/router/environment-page/resources/router.ts (3)
1-20
: Imports are suitable and typed
All required validators, schema, and utilities are referenced properly. If_
from lodash is not utilized extensively, consider dropping it to keep imports lean.
55-76
: Resource query with limit/offset
This dual-stage approach (one query for resources, another for job triggers) is well-structured. For very large datasets, you might need additional indexes or a separate approach to resource filtering alongside job triggers to optimize performance.
78-105
: Job triggers retrieval
By not limiting this query with the same pagination, you’re retrieving all relevant job triggers for the returned resources. That might be correct for accurate health status. Just remain aware of potential performance overhead if the resource set is large.apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/resources/ResourcesPageContent.tsx (1)
422-424
: Consider adding resource status to ResourceCard.The ResourceCard component is being used without explicitly passing the resourceStatus prop, which may be needed based on the component's implementation.
-<ResourceCard key={resource.id} resource={resource} /> +<ResourceCard + key={resource.id} + resource={resource} + resourceStatus={resource.status} +/>
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/deployments/EnvironmentDeploymentsPageContent.tsx
(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/deployments/_components/StatusBadge.tsx
(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/deployments/page.tsx
(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/overview/page.tsx
(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/resources/ResourcesPageContent.tsx
(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/resources/_hooks/useFilteredResources.ts
(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/resources/page.tsx
(1 hunks)packages/api/src/router/environment-page/deployments/router.ts
(1 hunks)packages/api/src/router/environment-page/overview/router.ts
(1 hunks)packages/api/src/router/environment-page/resources/router.ts
(1 hunks)packages/api/src/router/environment-page/router.ts
(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/api/src/router/environment-page/router.ts
- apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/overview/page.tsx
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{ts,tsx}`: **Note on Error Handling:** Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error...
**/*.{ts,tsx}
: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/resources/ResourcesPageContent.tsx
packages/api/src/router/environment-page/overview/router.ts
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/deployments/page.tsx
packages/api/src/router/environment-page/resources/router.ts
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/resources/_hooks/useFilteredResources.ts
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/resources/page.tsx
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/deployments/_components/StatusBadge.tsx
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/deployments/EnvironmentDeploymentsPageContent.tsx
packages/api/src/router/environment-page/deployments/router.ts
🧬 Code Definitions (6)
packages/api/src/router/environment-page/overview/router.ts (3)
packages/api/src/router/environment-page/overview/deployment-stats.ts (1)
getDeploymentStats
(36-117)packages/api/src/router/environment-page/overview/version-distro.ts (1)
getVersionDistro
(16-67)packages/api/src/router/environment-page/overview/desired-version.ts (1)
getDesiredVersion
(7-104)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/deployments/page.tsx (2)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/_deployments/page.tsx (1)
DeploymentsPage
(3-13)apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/deployments/EnvironmentDeploymentsPageContent.tsx (1)
EnvironmentDeploymentsPageContent
(34-255)
packages/api/src/router/environment-page/resources/router.ts (2)
packages/validators/src/resources/conditions/resource-condition.ts (2)
resourceCondition
(38-48)ResourceCondition
(27-36)packages/validators/src/jobs/index.ts (3)
analyticsStatuses
(46-52)failedStatuses
(54-59)activeStatus
(32-32)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/resources/_hooks/useFilteredResources.ts (1)
packages/validators/src/resources/conditions/resource-condition.ts (1)
ResourceCondition
(27-36)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/resources/page.tsx (2)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/_resources/page.tsx (1)
ResourcesPage
(6-21)apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/resources/ResourcesPageContent.tsx (1)
ResourcesPageContent
(172-470)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/deployments/EnvironmentDeploymentsPageContent.tsx (1)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/deployments/_components/StatusBadge.tsx (1)
StatusBadge
(3-58)
🪛 Biome (1.9.4)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/resources/ResourcesPageContent.tsx
[error] 435-435: Comparing to itself is potentially pointless.
(lint/suspicious/noSelfCompare)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build (linux/amd64)
- GitHub Check: Typecheck
- GitHub Check: Lint
🔇 Additional comments (24)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/deployments/page.tsx (2)
17-19
: Good practice verifying workspace existence.The
if (workspace == null) return notFound();
block is a straightforward and clear approach to handle the scenario of an invalid slug, preventing downstream errors. This also aligns with Next.js best practices for handling "not found" routes.
22-33
: Ensure consistent error handling for other missing data.Currently, you handle a missing
workspace
but do not handle a missing or invalidenvironmentId
scenario. Confirm that upstream code ensures a valid environment or consider adding a safeguard here if needed.packages/api/src/router/environment-page/overview/router.ts (1)
1-177
: Overall, well-structured usage of TRPC and database queries.Apart from the missing null checks, the rest of the code is well-organized and consistent with best practices.
packages/api/src/router/environment-page/deployments/router.ts (1)
1-259
: Great use of composable helper functions and TRPC router architecture.Overall, the code is cohesive and logical.
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/resources/_hooks/useFilteredResources.ts (4)
1-2
: No issues with the 'use client' directive.
This is a standard Next.js pattern for client-side components.
3-4
: Imports look good
UsingResourceCondition
ensures you have strong typing for the optional filter parameter.
7-13
: Documentation is clear and concise
The JSDoc adequately explains each parameter and the return type.
21-24
:❓ Verification inconclusive
Verify strict checking for empty environment ID
Using{ enabled: environmentId !== "" }
disables the query whenenvironmentId
is empty. Confirm whether you also need to handlenull
orundefined
if environment data is being fetched asynchronously.
🏁 Script executed:
#!/bin/bash # Searching references where environmentId might be null or undefined in the codebase rg "environmentId\s*=\s*.*" -A 3Length of output: 16915
Attention: Validate Handling of Potential Null/Undefined Environment IDs
Currently, the query is enabled only when
environmentId !== ""
. However, if environment data is fetched asynchronously, there’s a risk thatenvironmentId
could benull
orundefined
, which wouldn’t be caught by this check. Please verify if additional conditions are necessary to also disable the query in these scenarios. For example, consider updating the condition to something like:{ enabled: environmentId != null && environmentId !== "" }or simply:
{ enabled: Boolean(environmentId) }Ensure that this change aligns with how environment identifiers are handled elsewhere in the codebase.
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/resources/page.tsx (4)
3-9
: Card imports
Importing cards from@ctrlplane/ui/card
cleanly structures the UI. No issues found.
12-12
: New import of ResourcesPageContent
Pulling inResourcesPageContent
is a sensible reorganization of the resource management UI.
40-51
: Card-based layout with ResourcePageContent
This approach maintains a clean, consistent structure and keeps environment details consolidated. You could consider an error boundary or loading state within the card ifResourcesPageContent
depends on async data.
17-21
:❓ Verification inconclusive
Good environment and workspace fetch logic
You handle missing environment/workspace usingnotFound()
. Consider verifying thatenvironmentId
andworkspaceSlug
are non-empty in edge cases.
🏁 Script executed:
#!/bin/bash # Searching for any potential usage of environmentId or workspaceSlug as empty strings rg "workspaceSlug\s*=\s*.*|environmentId\s*=\s*.*" -A 3Length of output: 23084
Action: Verify Parameter Emptiness
The logic for fetching the workspace and environment is solid with proper handling via
notFound()
. However, there’s a potential edge-case risk: ifenvironmentId
orworkspaceSlug
are received as empty strings, the subsequent API calls could behave unexpectedly. While our broader codebase shows similar parameters being used without explicit empty-string checks, it might be safer to add validation for non-empty values to maintain consistency and prevent accidental misuse.• If an empty string is a possibility, consider adding checks (e.g.,
if (!environmentId || !workspaceSlug) return notFound()
) before proceeding with further API calls.packages/api/src/router/environment-page/resources/router.ts (5)
21-23
: Router creation
createTRPCRouter
usage is clear and standard.
34-39
: Authorization checks
This is a good use ofmeta
to ensure only authorized users can retrieve environment resources.
47-53
: Combining filters
Mergingenvironment.resourceFilter
withinput.filter
under an AND comparison is a solid pattern.
142-147
: Final resource mapping
The fallback tostatus: "healthy"
andsuccessRate: 0
is sensible as long as the preceding logic ensures accurate classification for unhealthy or actively deploying states. After fixing the object mismatch, this mapping will properly reflect resource health.
24-33
:❓ Verification inconclusive
Validate pagination defaults and bounds
A default limit of 500 and max of 1000 is reasonable. Ensure these limits align with your performance constraints.
🏁 Script executed:
#!/bin/bash # Searching for usage of environment.page.resources.list in the codebase to see if limit can exceed 1000 rg "environment\.page\.resources\.list(\s|\()"Length of output: 48
Ensure Pagination Defaults and Bounds Align with Performance Constraints
The code in
packages/api/src/router/environment-page/resources/router.ts
correctly validates the pagination inputs (limit between 1 and 1000 with a default of 500, and a non-negative offset). However, the automated search for usages ofenvironment.page.resources.list
returned no results. Please manually verify that:
- This endpoint isn’t invoked elsewhere in unexpected ways that might bypass these validations.
- The chosen limits (default 500 and maximum 1000) meet the application's performance constraints.
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/resources/ResourcesPageContent.tsx (7)
48-56
: Good use of robust error handling for parseInt.The
safeParseInt
function properly handles invalid input cases and ensures the parsed page number is within valid bounds. This prevents potential errors when dealing with URL parameters.
58-68
: Well-implemented pagination hook with URL persistence.The
usePagination
hook elegantly manages pagination state through URL parameters, allowing for shareable links and browser history navigation. This is a good pattern for maintaining state across page refreshes.
171-217
: LGTM! Well-structured filtering logic with composable filter conditions.The component effectively combines environment filters with user-created filters, allowing for granular resource filtering. The pagination implementation is also properly integrated with the filtering system.
241-247
: Good use of lodash for resource grouping.The implementation cleanly groups resources by version and kind, making it easy to populate filter dropdowns and organize data.
231-239
: Good use of debouncing for search input.The search functionality correctly implements debouncing to prevent excessive filter updates while the user is typing, which improves performance and user experience.
315-416
: Well-designed search and filter UI with intuitive controls.The search and filter section provides multiple ways for users to refine their view (text search, dropdowns for kinds/versions, and a filter dialog). The layout is responsive and includes a toggle for switching between grid and list views.
252-313
: Good implementation of resource summary cards with dynamic statistics.The summary cards effectively display resource statistics with appropriate visual indicators for different statuses. The dynamic calculation of percentages and status counts provides useful insights at a glance.
.query(async ({ ctx, input }) => { | ||
const environment = await ctx.db | ||
.select() | ||
.from(SCHEMA.environment) | ||
.where(eq(SCHEMA.environment.id, input.environmentId)) | ||
.then(takeFirst); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Environment existence check
The code fetches the environment but does not handle the scenario if takeFirst
returns undefined. Consider explicitly returning an error or an empty result if the environment is missing.
const environment = await ctx.db
.select()
.from(SCHEMA.environment)
.where(eq(SCHEMA.environment.id, input.environmentId))
.then(takeFirst);
-if (!environment) {
- // environment not found, consider returning an error or throwing
-}
Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
take first will return the error if its not found
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the definition of takeFirst
rg "export const takeFirst" --type ts
# Or its declaration
rg "takeFirst.*=" --type ts
# Let's also check for any import statements for takeFirst
rg "import.*takeFirst" --type ts
Length of output: 6492
🏁 Script executed:
#!/bin/bash
# Let's check the implementation of takeFirst
cat packages/db/src/common.ts | grep -A 10 "export const takeFirst = <T"
Length of output: 406
@adityachoudhari26 You're absolutely right. Looking at the implementation of takeFirst
in packages/db/src/common.ts
:
export const takeFirst = <T extends any[]>(values: T): T[number] => {
if (values.length !== 1)
throw new Error("Found non unique or inexistent value");
return values[0]!;
};
It explicitly throws an error with the message "Found non unique or inexistent value" when the array is empty (no record found) or contains multiple records. This means the additional error check suggested in the review comment is indeed unnecessary since takeFirst
already handles this case by throwing an error.
✏️ Learnings added
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#395
File: packages/api/src/router/environment-page/resources/router.ts:40-45
Timestamp: 2025-03-24T18:46:38.743Z
Learning: The `takeFirst` utility function in the codebase (from `@ctrlplane/db`) throws an Error with message "Found non unique or inexistent value" if the result array doesn't contain exactly one element, making additional null/undefined checks unnecessary after its use.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (5)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/overview/_components/DeploymentTelemetryTable.tsx (4)
25-41
: Consider capturing the error state from TRPC queries for resilience.Currently, the
api.environment.page.overview.telemetry.byDeploymentId.useQuery
call fetches data usingdata
andisLoading
but does not handle or display error states. Capturing theerror
from the query would allow you to show a user-friendly message when something goes wrong, which improves overall user experience.- const { data: telemetry, isLoading } = + const { data: telemetry, isLoading, error } = api.environment.page.overview.telemetry.byDeploymentId.useQuery(...); + if (error) { + return <div className="py-3 text-red-500">Failed to load data</div>; + }
80-83
: Provide a more descriptive empty distribution state.Instead of rendering only an empty bar when both distributions are empty, consider clarifying the possible reasons (e.g., “No active deployments or distribution data is unavailable”) to help users understand why no telemetry data is shown.
208-230
: Add a fallback for unrecognized deployment statuses.You cover specific statuses with color-coded labels but do not include a default case for unexpected or new statuses. Consider adding a fallback label (e.g., “Unknown”) and color mapping to avoid rendering blank badges if the status is not recognized.
"inline-flex items-center gap-1.5 rounded px-2 py-1 text-xs font-medium", { "bg-green-500/10 text-green-400": desiredVersion.status === "Deployed", "bg-blue-500/10 text-blue-400": desiredVersion.status === "Deploying", "bg-red-500/10 text-red-400": desiredVersion.status === "Failed", "bg-amber-500/10 text-amber-400": desiredVersion.status === "Pending Approval", + "bg-neutral-500/10 text-neutral-400": ![ + "Deployed", + "Deploying", + "Failed", + "Pending Approval", + ].includes(desiredVersion.status), },
156-159
: Replace hardcoded color for the deployment name dot with dynamic data or status indication.At the moment, the small dot next to the deployment name is always green (
bg-green-500
), which might be misleading if the deployment is in another state (e.g., failed, deploying, etc.). Use a condition or a status field to determine the correct color.apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/resources/ResourcesPageContent.tsx (1)
172-218
: Initial resource stats queries.
CallinguseFilteredResources
twice (for totals and for paginated data) is acceptable but may lead to higher load. Consider merging these queries if performance becomes a concern.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/overview/_components/DeploymentTelemetryTable.tsx
(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/resources/ResourcesPageContent.tsx
(1 hunks)packages/api/src/router/environment-page/resources/router.ts
(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{ts,tsx}`: **Note on Error Handling:** Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error...
**/*.{ts,tsx}
: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
packages/api/src/router/environment-page/resources/router.ts
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/resources/ResourcesPageContent.tsx
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/overview/_components/DeploymentTelemetryTable.tsx
🧠 Learnings (1)
packages/api/src/router/environment-page/resources/router.ts (1)
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#395
File: packages/api/src/router/environment-page/resources/router.ts:40-45
Timestamp: 2025-03-24T18:46:38.894Z
Learning: The `takeFirst` utility function in the codebase (from `@ctrlplane/db`) throws an Error with message "Found non unique or inexistent value" if the result array doesn't contain exactly one element, making additional null/undefined checks unnecessary after its use.
🧬 Code Definitions (2)
packages/api/src/router/environment-page/resources/router.ts (2)
packages/validators/src/resources/conditions/resource-condition.ts (2)
resourceCondition
(38-48)ResourceCondition
(27-36)packages/validators/src/jobs/index.ts (3)
analyticsStatuses
(46-52)failedStatuses
(54-59)activeStatus
(32-32)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/resources/ResourcesPageContent.tsx (5)
packages/validators/src/resources/conditions/resource-condition.ts (1)
ResourceCondition
(27-36)packages/validators/src/resources/conditions/comparison-condition.ts (1)
ComparisonCondition
(45-60)apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/resources/_hooks/useFilteredResources.ts (1)
useFilteredResources
(14-26)apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/resources/_components/ResourceCard.tsx (1)
ResourceCard
(42-110)apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/resources/_components/ResourceTable.tsx (1)
ResourceTable
(79-119)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Typecheck
- GitHub Check: Lint
- GitHub Check: build (linux/amd64)
🔇 Additional comments (21)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/overview/_components/DeploymentTelemetryTable.tsx (1)
137-141
: Revisit API error handling approach.A previous review comment recommended adding error handling for this API query. You might have reasons for not implementing it, but that guidance is still valid to improve user experience if data fetching fails.
packages/api/src/router/environment-page/resources/router.ts (8)
1-22
: Imports and initial setup look good.
No correctness or maintainability concerns are apparent here.
23-39
: Appropriate use of TRPC router structure and authorization metadata.
The.input
validation and the creation of alist
procedure are well-organized.
40-46
: Confirming no environment check needed.
SincetakeFirst
throws if the environment does not exist or isn't unique, no further checks are necessary. This is consistent with the retrieved learnings.
47-53
: Comprehensive resource filter construction.
Mergingenvironment.resourceFilter
with the user-provided conditions is a clean approach.
55-76
: Efficient resource retrieval with joins and pagination.
The code appropriately filters out soft-deleted resources and limits the row set with offset & limit.
78-105
: Job trigger queries look correct.
Fetching the distinct relevant rows for resource job triggers in a single query is optimal and avoids N+1 queries.
107-142
: Logical health status computation.
The code accurately checks for unhealthy statuses first, then “deploying,” defaulting to “healthy.” This approach is straightforward and easy to maintain.
144-149
: Concise final return.
Merging the original resource data with computed health metrics is well done.apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/resources/ResourcesPageContent.tsx (12)
1-19
: Import statements and initial setup appear consistent.
Importing both local hooks and UI components is well-structured.
20-46
: Constants, condition types, and approach align with the codebase patterns.
No issues found with the continuous usage of resource types and logical setup.
48-56
: Robust integer parsing and boundary checks.
ThesafeParseInt
function uses fallback logic for invalid pages, which helps avoid unexpected crashes on invalid queries.
58-68
: Pagination hook is correctly implemented.
Managing URL search parameters ensures the user’s page navigation state persists properly.
70-84
: FunctionalparseResourceFilter
approach.
Wrapping single field conditions inside a top-level comparison is logical.
86-129
: Dropdown filter logic is well-factored.
The function removes or adds conditions, avoiding duplication.
130-170
: Search-based filter construction is consistent.
Parsing the search term into a name-based filter is straightforward and easily extensible.
219-240
: Dynamic filter merges for user-driven changes.
Properly constructs a final filter by combining environment and local conditions.
241-312
: Summary cards correctly derived from resource data.
Counts and percentage calculations are straightforward and user-friendly.
315-416
: Clear and intuitive UI for search and advanced filters.
Grid/list toggles and filter dialogs align with typical user interface conventions.
418-432
: Grid and list views handle isLoading state gracefully.
Skeleton placeholders during loading enhance user experience.
433-470
: Pagination and filtered resource display.
Conditionally showing total vs. paginated counts is well-implemented, and “Filtered” label clarifies user-applied criteria.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🧹 Nitpick comments (7)
packages/api/src/router/environment-page/deployments/router.ts (3)
112-214
: Consider breaking downgetDeploymentStats
into smaller functions.While the implementation is thorough and handles various aspects well, this function is quite long and performs multiple database operations. Consider decomposing it into smaller, focused functions for improved maintainability.
The function could be split into separate concerns:
- Fetching version and approval data
- Retrieving and filtering resources
- Getting job information
- Computing statistics
This would improve readability and make testing individual parts easier.
256-282
: Consider extracting sorting logic to a separate function.The sorting logic contains multiple conditional blocks that could be extracted to a separate function for better readability and maintainability. This would make the main query function more focused.
- deploymentStats - .then((stats) => stats.filter(isPresent)) - .then((stats) => { - if (orderBy == "recent") - return stats.sort( - (a, b) => b.deployedAt.getTime() - a.deployedAt.getTime(), - ); - if (orderBy == "oldest") - return stats.sort( - (a, b) => a.deployedAt.getTime() - b.deployedAt.getTime(), - ); - if (orderBy == "duration") - return stats.sort((a, b) => b.duration - a.duration); - if (orderBy == "success") - return stats.sort((a, b) => b.successRate - a.successRate); - return stats; - }); + deploymentStats + .then((stats) => stats.filter(isPresent)) + .then((stats) => sortDeploymentStats(stats, orderBy)); + const sortDeploymentStats = ( + stats: Array<NonNullable<Awaited<ReturnType<typeof getDeploymentStats>>>>, + orderBy?: string + ) => { + if (orderBy === "recent") { + return stats.sort( + (a, b) => b.deployedAt.getTime() - a.deployedAt.getTime() + ); + } + if (orderBy === "oldest") { + return stats.sort( + (a, b) => a.deployedAt.getTime() - b.deployedAt.getTime() + ); + } + if (orderBy === "duration") { + return stats.sort((a, b) => b.duration - a.duration); + } + if (orderBy === "success") { + return stats.sort((a, b) => b.successRate - a.successRate); + } + return stats; + };
216-286
: Consider adding pagination for better performance with large datasets.The current implementation fetches all deployments without pagination. If the number of deployments grows large, this could lead to performance issues. Consider implementing pagination for the
list
procedure.Add pagination parameters to the input schema and update the query to limit the results accordingly:
.input( z.object({ environmentId: z.string().uuid(), workspaceId: z.string().uuid(), search: z.string().optional(), status: z .enum(["pending", "failed", "deploying", "success"]) .optional(), orderBy: z.enum(["recent", "oldest", "duration", "success"]).optional(), + limit: z.number().min(1).max(100).default(20), + offset: z.number().min(0).default(0), }), )And update the return structure to include pagination metadata:
return deploymentStats; + return { + items: deploymentStats, + pagination: { + total: deploymentStats.length, // This would need to be the total count without limit + limit: input.limit, + offset: input.offset, + } + };apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/deployments/EnvironmentDeploymentsPageContent.tsx (4)
132-196
: Consider removing or implementing the commented metrics cards.There's a large block of commented-out code for deployment metrics cards. While keeping it as a reference might be helpful during development, it should either be implemented or removed before final deployment.
- {/* Deployment Summary Cards */} - {/* <div className="mb-6 grid grid-cols-1 gap-4 md:grid-cols-4"> - <div className="rounded-lg border border-neutral-800 bg-neutral-900/50 p-4"> - <div className="flex items-center justify-between"> - <div className="text-xs text-neutral-400">Total Deployments</div> - <div className="rounded-full bg-neutral-800/50 px-2 py-1 text-xs text-neutral-300"> - Last 30 days - </div> - </div> - <div className="mt-2 text-2xl font-semibold text-neutral-100">42</div> - <div className="mt-1 flex items-center text-xs text-green-400"> - <span>↑ 8% from previous period</span> - </div> - </div> - - <div className="rounded-lg border border-neutral-800 bg-neutral-900/50 p-4"> - <div className="flex items-center justify-between"> - <div className="text-xs text-neutral-400">Success Rate</div> - <div className="rounded-full bg-neutral-800/50 px-2 py-1 text-xs text-neutral-300"> - Last 30 days - </div> - </div> - <div className="mt-2 text-2xl font-semibold text-green-400"> - 89.7% - </div> - <div className="mt-1 flex items-center text-xs text-green-400"> - <span>↑ 3.2% from previous period</span> - </div> - <div className="mt-2 h-1.5 w-full rounded-full bg-neutral-800"> - <div - className="h-full rounded-full bg-green-500" - style={{ width: "89.7%" }} - ></div> - </div> - </div> - - <div className="rounded-lg border border-neutral-800 bg-neutral-900/50 p-4"> - <div className="flex items-center justify-between"> - <div className="text-xs text-neutral-400">Avg. Duration</div> - <div className="rounded-full bg-neutral-800/50 px-2 py-1 text-xs text-neutral-300"> - Last 30 days - </div> - </div> - <div className="mt-2 text-2xl font-semibold text-neutral-100"> - 3m 42s - </div> - <div className="mt-1 flex items-center text-xs text-red-400"> - <span>↑ 12% from previous period</span> - </div> - </div> - - <div className="rounded-lg border border-neutral-800 bg-neutral-900/50 p-4"> - <div className="flex items-center justify-between"> - <div className="text-xs text-neutral-400">Deployment Frequency</div> - <div className="rounded-full bg-neutral-800/50 px-2 py-1 text-xs text-neutral-300"> - Last 30 days - </div> - </div> - <div className="mt-2 text-2xl font-semibold text-neutral-100"> - 1.4/day - </div> - <div className="mt-1 flex items-center text-xs text-green-400"> - <span>↑ 15% from previous period</span> - </div> - </div> - </div> */}
76-95
: Add accessibility attributes to the progress bar.The success rate progress bar is visually informative but could benefit from accessibility enhancements.
<div className="flex items-center gap-2"> <div className="h-1.5 w-16 rounded-full bg-neutral-800"> <div + role="progressbar" + aria-valuenow={Number(deploymentStat.successRate * 100)} + aria-valuemin={0} + aria-valuemax={100} className={`h-full rounded-full ${ deploymentStat.successRate * 100 > 90 ? "bg-green-500" : deploymentStat.successRate * 100 > 70 ? "bg-amber-500" : "bg-red-500" }`} style={{ width: `${Number(deploymentStat.successRate * 100)}%`, }} /> </div> <span className="text-sm"> {Number(deploymentStat.successRate * 100).toFixed(1)}% </span> </div>
53-104
: Add click handler to the deployment rows.The table rows have
cursor-pointer
styling, suggesting they should be clickable, but there's no click handler implemented.<TableRow key={deploymentStat.deployment.id} className="h-12 cursor-pointer border-b border-neutral-800/50 hover:bg-neutral-800/20" + onClick={() => { + // Navigate to deployment details page + window.location.href = `/path/to/deployment/${deploymentStat.deployment.id}`; + // Or with Next.js router: + // router.push(`/path/to/deployment/${deploymentStat.deployment.id}`); + }} >
275-286
: Add empty state for when no deployments are found.Currently, there's no handling for when the deployments list is empty. Consider adding a message to inform users when no results match their search/filter criteria.
<TableBody> {deploymentStatsQ.isLoading && Array.from({ length: 3 }).map((_, index) => ( <SkeletonRow key={index} /> ))} + {!deploymentStatsQ.isLoading && deploymentStats.length === 0 && ( + <TableRow> + <TableCell colSpan={8} className="h-32 text-center text-neutral-400"> + No deployments found matching your criteria. + </TableCell> + </TableRow> + )} {deploymentStats.map((deploymentStat) => ( <DeploymentRow key={deploymentStat.deployment.id} deploymentStat={deploymentStat} /> ))} </TableBody>
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/deployments/EnvironmentDeploymentsPageContent.tsx
(1 hunks)packages/api/src/router/environment-page/deployments/router.ts
(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{ts,tsx}`: **Note on Error Handling:** Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error...
**/*.{ts,tsx}
: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/deployments/EnvironmentDeploymentsPageContent.tsx
packages/api/src/router/environment-page/deployments/router.ts
🧬 Code Definitions (1)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/deployments/EnvironmentDeploymentsPageContent.tsx (1)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/deployments/_components/StatusBadge.tsx (1)
StatusBadge
(3-58)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build (linux/amd64)
- GitHub Check: Typecheck
- GitHub Check: Lint
🔇 Additional comments (10)
packages/api/src/router/environment-page/deployments/router.ts (8)
1-29
: Well structured imports and clean organization.The imports are well-organized with clear grouping of external libraries first, followed by internal dependencies. Good use of named imports for specific functions from database utilities.
31-59
: Good database query implementation for version selector.The
getVersionSelector
function is well structured with proper join conditions and filtering. The use oftakeFirstOrNull
and fallback to null with the nullish coalescing operator is a robust approach for handling missing data.
61-76
: Effective status determination with clear priority logic.The status determination logic follows a clear priority sequence (failed -> pending -> success -> deploying) that makes the code easy to understand and maintain. The early returns improve readability.
78-96
: Accurate duration calculation with proper edge case handling.The function correctly filters out skipped or cancelled jobs and handles null timestamps appropriately. The use of lodash's
minBy
andmaxBy
is an elegant way to find the time boundaries.
98-110
: Clean implementation of success rate calculation.The function properly handles the edge case of no completed jobs and uses clear filtering logic to determine successful jobs. The calculation is straightforward and easy to understand.
219-227
: Good input validation with comprehensive options.The input schema is well-defined with proper types and validation. The optional parameters for search, status filtering, and ordering provide good flexibility for the API client.
229-234
: Proper authorization check implementation.The authorization meta configuration ensures users have the necessary permissions before accessing environment data, following security best practices.
247-254
:❓ Verification inconclusive
Verify the behavior of
search
with empty strings.The search condition uses
ilike
with%${search}%
, but doesn't account for what happens if search is an empty string. Empty strings with%
wildcards can match everything, which might not be the intended behavior.
🏁 Script executed:
#!/bin/bash # Check if there are any cases where empty search strings are handled specially rg -A 3 $'ilike.*search' --glob "*.ts" | grep -E "(search\s*===?\s*''|search\.length)"Length of output: 93
Action Required: Confirm Intended Behavior for Empty Search Values
After verifying the code with our search commands, there appears to be no explicit handling for when the
search
parameter is an empty string. In the current implementation withinpackages/api/src/router/environment-page/deployments/router.ts
(lines 247-254), ifsearch
is an empty string, the conditionsearch != null ? ilike(SCHEMA.deployment.name, `%${search}%`) : undefined,evaluates to
ilike(SCHEMA.deployment.name, '%%')
, which will match all deployments. This behavior may be unintended if the goal is to filter results only when a meaningful search string is provided.
- File: packages/api/src/router/environment-page/deployments/router.ts
- Location: Lines 247-254
- Observation: The code lacks a check for empty strings, resulting in a wildcard search that potentially returns all entries.
Please confirm whether this behavior is acceptable or if a guard should be added to ensure that an empty search string does not trigger an all-encompassing match.
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/deployments/EnvironmentDeploymentsPageContent.tsx (2)
109-119
: Props are now properly used.Nice implementation! I see you're now correctly using both the
environmentId
andworkspaceId
props that are passed to the component. This resolves the previous issue whereenvironmentId
was accepted but not used.
121-127
: Good use of TRPC for data fetching.The implementation properly uses TRPC for fetching data with the right parameters and includes placeholder data to prevent UI flickering. This is a nice improvement over the previous static data approach.
...emSlug]/(raw)/environments/[environmentId]/deployments/EnvironmentDeploymentsPageContent.tsx
Show resolved
Hide resolved
{deploymentStats.map((deploymentStat) => ( | ||
<DeploymentRow | ||
key={deploymentStat.deployment.id} | ||
deploymentStat={deploymentStat} | ||
/> | ||
))} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Verification agent
🧩 Analysis chain
Consider adding pagination for large deployment lists.
The component currently displays all deployments without pagination, which could cause performance issues with large datasets. Consider implementing pagination.
🏁 Script executed:
#!/bin/bash
# Check if the API endpoint supports pagination
# Let's see if there are any pagination parameters in the API router for deployments
rg -A 5 "environment\.page\.deployments\.list" --glob "*.{ts,tsx}"
Length of output: 1325
Action Required: Implement UI-Level Pagination
The API call (api.environment.page.deployments.list.useQuery
) indicates that the backend supports pagination, but the component currently renders all deployment rows at once. This approach could become a performance bottleneck with large datasets. Please consider adding UI pagination controls (or infinite scroll) and update the query parameters accordingly to leverage the paginated API.
- File:
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/deployments/EnvironmentDeploymentsPageContent.tsx
- Code snippet:
{deploymentStats.map((deploymentStat) => ( <DeploymentRow key={deploymentStat.deployment.id} deploymentStat={deploymentStat} /> ))}
- Ensure that pagination parameters (such as page number and page size) are recognized by the API and are integrated into the UI logic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🧹 Nitpick comments (12)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/deployments/_components/summary-card/DeploymentFrequency.tsx (2)
10-19
: Consider optimizing duplicate data fetching.This component uses the same query (api.environment.page.deployments.total) as the TotalDeployments component. If these components are used together on the same page, consider lifting the data fetching to a parent component to avoid duplicate requests.
21-22
: Hardcoded period length assumption.The calculation divides by 30, assuming a 30-day period. Consider making this more flexible by either:
- Getting the actual period length from the API response
- Adding a period length prop to the component
This would make the component more maintainable if the period changes in the future.
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/deployments/_components/summary-card/AverageDuration.tsx (3)
57-60
: Remove console.log statementsThere's a debugging console.log statement that should be removed before production.
- console.log({ - averageDurationInCurrentPeriod, - averageDurationInPreviousPeriod, - });
40-50
: Add loading and error state handlingThe component doesn't handle loading or error states from the TRPC query, which could lead to a poor user experience if the data is slow to load or if there's an error.
Add loading and error state handling:
const averageDurationQ = api.environment.page.deployments.aggregateStats.useQuery({ environmentId, workspaceId, }); + if (averageDurationQ.isLoading) { + return ( + <div className="rounded-lg border border-neutral-800 bg-neutral-900/50 p-4"> + <div className="animate-pulse h-24"></div> + </div> + ); + } + + if (averageDurationQ.isError) { + return ( + <div className="rounded-lg border border-neutral-800 bg-neutral-900/50 p-4"> + <div className="text-xs text-red-400">Failed to load data</div> + </div> + ); + } const { statsInCurrentPeriod, statsInPreviousPeriod } = averageDurationQ.data ?? { statsInCurrentPeriod: { averageDuration: 0 }, statsInPreviousPeriod: { averageDuration: 0 }, };
74-77
: Handle edge case where duration is zeroThe current implementation doesn't specifically handle the case where
averageDurationInCurrentPeriodMs
is zero, which might lead to confusing display.Consider adding a check for zero duration:
-{prettyMilliseconds(averageDurationInCurrentPeriodMs, { - secondsDecimalDigits: 0, -})} -{/* 0ms */} +{averageDurationInCurrentPeriodMs === 0 + ? "0ms" + : prettyMilliseconds(averageDurationInCurrentPeriodMs, { + secondsDecimalDigits: 0, + }) +}packages/api/src/router/environment-page/deployments/deployment-stats.ts (3)
48-63
: Approval logic might conflict with job statuses.
If the approval’s status ispending
, the function immediately returns"pending"
even if there are failed or successful jobs. Confirm downstream logic aligns with this precedence.
65-83
: Clarify duration unit in function name or documentation.
The functiongetDuration
currently calculates a millisecond difference but returns it as a raw number. Consider documenting or renaming to reflect that it’s in milliseconds for clarity.
99-202
: Possible performance concern with multiple sequential queries.
This function makes multiple DB queries for the row, resources, and jobs. If the environment has many resources, consider a more optimized or batched approach. That said, the logic is otherwise straightforward and correct.packages/api/src/router/environment-page/deployments/router.ts (2)
26-94
: Consider potential performance issues with large deployments.
Promise.all
ofgetDeploymentStats
can become expensive if there are many deployments. You might batch or paginate these calls to manage load effectively.
186-295
: Success rate is a percentage while duration is in seconds.
ThesuccessRate
calculation yields a percentage (multiplied by 100) whereasaverageDuration
is returned in numeric seconds (rounded). Document these distinct units for clarity.apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/deployments/EnvironmentDeploymentsPageContent.tsx (2)
57-60
: Consider removing "cursor-pointer" or adding an onClick handler.The table row is styled as clickable, but there is no onClick handler. If the row is not intended to be interactive, removing the cursor pointer can improve accessibility and clarity.
Here's a possible diff if you choose to remove the pointer style:
- className="h-12 cursor-pointer border-b border-neutral-800/50 hover:bg-neutral-800/20" + className="h-12 border-b border-neutral-800/50 hover:bg-neutral-800/20"
80-99
: Refactor color logic for success rate.The nested ternary operations make the styling conditions less clear. Consider extracting this color logic into a small utility function or mapping table for better maintainability.
For example, you could define a helper function:
function getSuccessRateColor(rate: number): string { if (rate > 0.9) return "bg-green-500"; if (rate > 0.7) return "bg-amber-500"; return "bg-red-500"; }And then in the JSX:
- className={`h-full rounded-full ${ - deploymentStat.successRate * 100 > 90 - ? "bg-green-500" - : deploymentStat.successRate * 100 > 70 - ? "bg-amber-500" - : "bg-red-500" - }`} + className={`h-full rounded-full ${getSuccessRateColor(deploymentStat.successRate)}`}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/deployments/EnvironmentDeploymentsPageContent.tsx
(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/deployments/_components/summary-card/AverageDuration.tsx
(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/deployments/_components/summary-card/DeploymentFrequency.tsx
(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/deployments/_components/summary-card/PercentChange.tsx
(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/deployments/_components/summary-card/SuccessRate.tsx
(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/deployments/_components/summary-card/TotalDeployments.tsx
(1 hunks)packages/api/src/router/environment-page/deployments/deployment-stats.ts
(1 hunks)packages/api/src/router/environment-page/deployments/router.ts
(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{ts,tsx}`: **Note on Error Handling:** Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error...
**/*.{ts,tsx}
: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/deployments/_components/summary-card/DeploymentFrequency.tsx
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/deployments/_components/summary-card/TotalDeployments.tsx
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/deployments/_components/summary-card/AverageDuration.tsx
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/deployments/_components/summary-card/SuccessRate.tsx
packages/api/src/router/environment-page/deployments/router.ts
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/deployments/EnvironmentDeploymentsPageContent.tsx
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/deployments/_components/summary-card/PercentChange.tsx
packages/api/src/router/environment-page/deployments/deployment-stats.ts
🧬 Code Definitions (5)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/deployments/_components/summary-card/DeploymentFrequency.tsx (2)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/deployments/_components/summary-card/PercentChange.tsx (1)
PercentChange
(8-31)apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/deployments/_components/summary-card/AverageDuration.tsx (1)
PercentChange
(12-34)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/deployments/_components/summary-card/AverageDuration.tsx (1)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/deployments/_components/summary-card/PercentChange.tsx (1)
PercentChange
(8-31)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/deployments/_components/summary-card/SuccessRate.tsx (2)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/deployments/_components/summary-card/PercentChange.tsx (1)
PercentChange
(8-31)apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/deployments/_components/summary-card/AverageDuration.tsx (1)
PercentChange
(12-34)
packages/api/src/router/environment-page/deployments/router.ts (1)
packages/api/src/router/environment-page/deployments/deployment-stats.ts (1)
getDeploymentStats
(99-201)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/deployments/EnvironmentDeploymentsPageContent.tsx (5)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/deployments/_components/StatusBadge.tsx (1)
StatusBadge
(3-58)apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/deployments/_components/summary-card/TotalDeployments.tsx (1)
TotalDeployments
(4-36)apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/deployments/_components/summary-card/SuccessRate.tsx (1)
SuccessRate
(6-66)apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/deployments/_components/summary-card/AverageDuration.tsx (1)
AverageDuration
(36-85)apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/deployments/_components/summary-card/DeploymentFrequency.tsx (1)
DeploymentFrequency
(6-41)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build (linux/amd64)
- GitHub Check: Typecheck
- GitHub Check: Lint
🔇 Additional comments (15)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/deployments/_components/summary-card/TotalDeployments.tsx (1)
1-36
: LGTM! Clean implementation of the TotalDeployments component.The component is well-structured, properly handles data loading states with default values, and presents the deployment statistics in a clear, user-friendly format. The use of PercentChange for showing trend data adds valuable context to the raw numbers.
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/deployments/_components/summary-card/PercentChange.tsx (2)
3-6
: LGTM! Good defensive coding.The helper function handles the division by zero case properly by returning 0 when the previous value is 0.
8-31
:❓ Verification inconclusive
Verify consistency in percentage change color coding across components.
This implementation uses green for positive changes and red for negative changes. However, the relevant snippet from AverageDuration.tsx shows opposite color coding (green for negative, red for positive). Ensure this is intentional based on whether increases or decreases are considered positive for each metric.
🏁 Script executed:
#!/bin/bash # Check if there are other components with potentially inconsistent color schemes grep -r "percentChange.*text-green" --include="*.tsx" . grep -r "percentChange.*text-red" --include="*.tsx" .Length of output: 997
Attention: Inconsistent Color Coding for Percentage Changes
It appears that the color associations differ between components. In
PercentChange.tsx
, a positive change renders with green (text-green-400
) and a negative change with red (text-red-400
), whereas inAverageDuration.tsx
, the logic is inverted. Please confirm whether these differences are intentional—reflecting distinct metric interpretations—or if they need to be standardized.
PercentChange.tsx:
- Positive change → green
- Negative change → red
AverageDuration.tsx:
- Positive change → red
- Negative change → green
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/deployments/_components/summary-card/DeploymentFrequency.tsx (1)
24-41
: LGTM! Clean presentation of deployment frequency.The component provides a clear visualization of deployment frequency with good formatting (one decimal place) and trend information.
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/deployments/_components/summary-card/SuccessRate.tsx (3)
26-32
: LGTM! Clear card header with consistent styling.The header follows the same pattern as the other summary cards, maintaining UI consistency.
33-44
: Good use of color coding based on success rate thresholds.The component appropriately uses color to convey the quality of the success rate (green for >90%, yellow for 50-90%, red for ≤50%).
45-63
: Excellent visual representation with progress bar.The addition of a progress bar provides an immediate visual cue of the success rate, enhancing the numeric display. The consistent color coding between text and progress bar reinforces the status indication.
packages/api/src/router/environment-page/deployments/deployment-stats.ts (3)
1-4
: Imports look good.
These imports are consistent with the rest of the codebase, and there are no obvious issues.
18-47
: Ensure handling of null version selectors in the caller.
ShouldgetVersionSelector
returnnull
, callers need to handle it gracefully before proceeding with database queries dependent on a version selector.
85-97
: Success rate returns a fractional value in [0, 1].
Double-check all call sites to ensure they correctly interpret the success rate. Some might mistakenly expect a percentage.packages/api/src/router/environment-page/deployments/router.ts (3)
96-117
: Authorization check logic is clear.
The check forenvironment == null
and then returningfalse
is concise. This approach prevents null-pointer errors when verifying if the user can list deployments.
120-146
: Distinct usage ensures correct counting.
UsingselectDistinctOn([resourceId, deploymentId])
is a neat way to avoid double-counting. Confirm that combining resourceId and deploymentId accurately captures unique deployments across resources.
148-178
: Edge case with partially deleted resources in previous period.
Theor
condition allows resource records that were deleted after the current period start. Confirm this is the desired logic if a resource was active only part of the previous period.apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/deployments/EnvironmentDeploymentsPageContent.tsx (2)
126-131
: Reinstate error handling and verifyplaceholderData
usage.It appears that no specific error handling is performed for
deploymentStatsQ.error
. This omission was flagged in a previous review. Additionally, verify whetherplaceholderData: (prev) => prev
is desired or if you'd prefer a different fallback mechanism.If this was removed unintentionally, you could reintroduce error handling similar to the earlier suggestion:
+ if (deploymentStatsQ.error) { + return ( + <div className="text-red-500"> + Failed to load deployments: {deploymentStatsQ.error.message} + </div> + ); + }And confirm that relying on previous query data is appropriate for your use case with
placeholderData: (prev) => prev
.
237-242
: Consider adding pagination for large datasets.This component renders all returned deployments. For performance on large datasets, implementing query-based pagination or virtualized scrolling can be crucial. This is a repeat suggestion from earlier reviews.
"mt-1 flex items-center text-xs", | ||
percentChange === 0 && "text-neutral-400", | ||
percentChange < 0 && "text-green-400", | ||
percentChange > 0 && "text-red-400", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Color logic is reversed for percentage changes
The color logic for the PercentChange
component is counterintuitive - it currently shows negative changes in green (line 22) and positive changes in red (line 23). This is the opposite of conventional UI patterns and inconsistent with the PercentChange
component in the separate file shared in the relevant code snippets.
Apply this fix to correct the color logic:
className={cn(
"mt-1 flex items-center text-xs",
percentChange === 0 && "text-neutral-400",
- percentChange < 0 && "text-green-400",
- percentChange > 0 && "text-red-400",
+ percentChange > 0 && "text-green-400",
+ percentChange < 0 && "text-red-400",
)}
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
"mt-1 flex items-center text-xs", | |
percentChange === 0 && "text-neutral-400", | |
percentChange < 0 && "text-green-400", | |
percentChange > 0 && "text-red-400", | |
className={cn( | |
"mt-1 flex items-center text-xs", | |
percentChange === 0 && "text-neutral-400", | |
percentChange > 0 && "text-green-400", | |
percentChange < 0 && "text-red-400", | |
)} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is because lower duration is better which is why its inverted for this specific one
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@adityachoudhari26 You're absolutely right. For the AverageDuration
component, the color logic is intentionally inverted because lower duration values are better - a decrease in average duration (negative percentage change) is a positive outcome that should be highlighted in green, while an increase in duration (positive percentage change) is a negative outcome that should be shown in red. This makes perfect sense for performance metrics like this.
✏️ Learnings added
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#395
File: apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/deployments/_components/summary-card/AverageDuration.tsx:20-23
Timestamp: 2025-03-25T18:00:35.183Z
Learning: For duration/time-based metrics like `AverageDuration`, the color logic for percentage changes is intentionally inverted compared to most other metrics: negative changes (decreases in duration) are shown in green because they represent improved performance, while positive changes (increases in duration) are shown in red because they represent degraded performance.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
export const PercentChange: React.FC<{ | ||
current: number; | ||
previous: number; | ||
}> = ({ current, previous }) => { | ||
const percentChange = getPercentChange(current, previous); | ||
return ( | ||
<div | ||
className={cn( | ||
"mt-1 flex items-center text-xs", | ||
percentChange === 0 && "text-neutral-400", | ||
percentChange < 0 && "text-green-400", | ||
percentChange > 0 && "text-red-400", | ||
)} | ||
> | ||
<span> | ||
{percentChange === 0 && "-"} | ||
{percentChange > 0 && "↑"} | ||
{percentChange < 0 && "↓"} {Number(percentChange).toFixed(0)}% from | ||
previous period | ||
</span> | ||
</div> | ||
); | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Remove duplicate component implementation
This PercentChange
component duplicates functionality that already exists in PercentChange.tsx
as shown in the relevant code snippets. Having two implementations of the same component creates maintenance challenges.
Consider importing and reusing the existing PercentChange
component rather than redefining it here:
- const getPercentChange = (current: number, previous: number) => {
- if (previous === 0) return 0;
- return ((current - previous) / previous) * 100;
- };
-
- export const PercentChange: React.FC<{
- current: number;
- previous: number;
- }> = ({ current, previous }) => {
- const percentChange = getPercentChange(current, previous);
- return (
- <div
- className={cn(
- "mt-1 flex items-center text-xs",
- percentChange === 0 && "text-neutral-400",
- percentChange < 0 && "text-green-400",
- percentChange > 0 && "text-red-400",
- )}
- >
- <span>
- {percentChange === 0 && "-"}
- {percentChange > 0 && "↑"}
- {percentChange < 0 && "↓"} {Number(percentChange).toFixed(0)}% from
- previous period
- </span>
- </div>
- );
- };
+ import { PercentChange } from "./PercentChange";
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
export const PercentChange: React.FC<{ | |
current: number; | |
previous: number; | |
}> = ({ current, previous }) => { | |
const percentChange = getPercentChange(current, previous); | |
return ( | |
<div | |
className={cn( | |
"mt-1 flex items-center text-xs", | |
percentChange === 0 && "text-neutral-400", | |
percentChange < 0 && "text-green-400", | |
percentChange > 0 && "text-red-400", | |
)} | |
> | |
<span> | |
{percentChange === 0 && "-"} | |
{percentChange > 0 && "↑"} | |
{percentChange < 0 && "↓"} {Number(percentChange).toFixed(0)}% from | |
previous period | |
</span> | |
</div> | |
); | |
}; | |
import { PercentChange } from "./PercentChange"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this one has inverted colors so we willkeep it separate
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@adityachoudhari26 I understand now - the colors are intentionally inverted in this implementation because for duration metrics, a decrease (negative change) is considered good (green) while an increase (positive change) is considered bad (red). This is different from other metrics where the color scheme might be reversed.
Keeping them separate makes sense since they have different semantic meanings despite the similar structure. Thanks for the clarification!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🧹 Nitpick comments (8)
packages/api/src/router/environment-page/overview/router.ts (2)
60-70
: Refactor resource fetching logic
Lines 60–70 and 139–149 both fetch resources from the database using very similar conditions. This duplication could be reduced and the code made more maintainable by placing the logic in a helper function (e.g.,fetchResources(ctx.db, filter, workspaceId)
) and using it in both locations.- const resources = await ctx.db - .select({ id: SCHEMA.resource.id }) - .from(SCHEMA.resource) - .where( - and( - isNull(SCHEMA.resource.deletedAt), - SCHEMA.resourceMatchesMetadata(ctx.db, environment.resourceFilter), - eq(SCHEMA.resource.workspaceId, workspaceId), - ), - ); - const resourceIds = await ctx.db - .select({ id: SCHEMA.resource.id }) - .from(SCHEMA.resource) - .where( - and( - SCHEMA.resourceMatchesMetadata(ctx.db, resourceSelector), - isNull(SCHEMA.resource.deletedAt), - ), - ) - .then((rs) => rs.map((r) => r.id)); + async function fetchResources(db: Tx, filter: ResourceCondition, workspaceId: string): Promise<string[]> { + const data = await db + .select({ id: SCHEMA.resource.id }) + .from(SCHEMA.resource) + .where( + and( + SCHEMA.resourceMatchesMetadata(db, filter), + eq(SCHEMA.resource.workspaceId, workspaceId), + isNull(SCHEMA.resource.deletedAt), + ), + ); + return data.map((r) => r.id); + }Also applies to: 139-149
71-78
: Be mindful of potential performance overhead with multiple deployment queries
Fetching every deployment and runninggetDeploymentStats(...)
for each might cause slowdowns when there are many deployments. Consider batching or aggregating these queries at the database level or limiting the total number of deployments if performance becomes an issue.apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/resources/_components/EditSelector.tsx (1)
13-37
: Implement loading state and add error handlingThe component correctly handles the environment update, but it lacks a loading indicator while the API request is in progress and doesn't handle potential errors from the mutation.
export const EditSelector: React.FC<{ environment: SCHEMA.Environment; }> = ({ environment }) => { const updateEnvironment = api.environment.update.useMutation(); const router = useRouter(); const onChange = (resourceCondition: ResourceCondition | null) => updateEnvironment .mutateAsync({ id: environment.id, data: { resourceFilter: resourceCondition ?? undefined }, }) .then(() => router.refresh()) + .catch((error) => { + // Handle error, e.g. display a toast notification + console.error("Failed to update environment:", error); + }); return ( <ResourceConditionDialog condition={environment.resourceFilter} onChange={onChange} > <Button size="sm" variant="outline" + disabled={updateEnvironment.isPending} > + {updateEnvironment.isPending ? "Updating..." : "Edit Selector"} </Button> </ResourceConditionDialog> ); };apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/resources/ResourcesPageContent.tsx (5)
190-201
: Consider using React.useMemo for derived state calculationsThese resource statistics are recalculated on every render but could be memoized for better performance.
- const totalResources = allResourcesQ.resources.length; - const healthyResources = allResourcesQ.resources.filter( - (r) => r.status === "healthy", - ).length; - const healthyPercentage = - totalResources > 0 ? (healthyResources / totalResources) * 100 : 0; - const unhealthyResources = allResourcesQ.resources.filter( - (r) => r.status === "unhealthy", - ).length; - const deployingResources = allResourcesQ.resources.filter( - (r) => r.status === "deploying", - ).length; + const { + totalResources, + healthyResources, + healthyPercentage, + unhealthyResources, + deployingResources + } = React.useMemo(() => { + const total = allResourcesQ.resources.length; + const healthy = allResourcesQ.resources.filter((r) => r.status === "healthy").length; + const healthyPct = total > 0 ? (healthy / total) * 100 : 0; + const unhealthy = allResourcesQ.resources.filter((r) => r.status === "unhealthy").length; + const deploying = allResourcesQ.resources.filter((r) => r.status === "deploying").length; + + return { + totalResources: total, + healthyResources: healthy, + healthyPercentage: healthyPct, + unhealthyResources: unhealthy, + deployingResources: deploying + }; + }, [allResourcesQ.resources]);
250-255
: Memoize resource grouping operationsThe resource grouping operations using Lodash are performed on every render, which could impact performance with large resource lists.
- // Group resources by component - const resourcesByVersion = _(resources) - .groupBy((t) => t.version) - .value() as Record<string, typeof resources>; - const resourcesByKind = _(resources) - .groupBy((t) => t.version + ": " + t.kind) - .value() as Record<string, typeof resources>; + // Group resources by component + const { resourcesByVersion, resourcesByKind } = React.useMemo(() => { + return { + resourcesByVersion: _(resources) + .groupBy((t) => t.version) + .value() as Record<string, typeof resources>, + resourcesByKind: _(resources) + .groupBy((t) => t.version + ": " + t.kind) + .value() as Record<string, typeof resources> + }; + }, [resources]);
478-491
: Add an empty state for when there are no resources to displayThe component doesn't handle the case where there are no resources after filtering. Consider adding a user-friendly message.
<div className="mt-4 flex items-center justify-between text-sm text-neutral-400"> <div> + {resources.length === 0 ? ( + <>No resources match your current filters</> + ) : {totalResources === resources.length ? ( <>Showing all {resources.length} resources</> ) : ( <> Showing {resources.length} of {totalResources} resources </> )} + } {resourceFilter != null && resourceFilter.conditions.length > 0 && ( <> {" "} • <span className="text-blue-400">Filtered</span> </> )} </div>
354-459
: Add a "Clear All Filters" button to improve usabilityWhen users apply multiple filters, it would be helpful to have a single button to clear all filters at once rather than requiring them to remove filters individually.
Add a Clear Filters button when filters are applied:
<div className="mb-4 flex flex-col justify-between gap-4 md:flex-row"> <div className="relative"> <IconSearch className="absolute left-3 top-1/2 h-4 w-4 -translate-y-1/2 text-muted-foreground" /> <Input placeholder="Search resources..." className="w-full pl-8 md:w-80" value={search} onChange={(e) => setSearch(e.target.value)} /> </div> <div className="flex flex-wrap items-center gap-2"> + {(resourceFilter != null && resourceFilter.conditions.length > 0 || search.length > 0) && ( + <Button + variant="outline" + onClick={() => { + setSearch(""); + setResourceFilter(null); + }} + > + Clear Filters + </Button> + )} <ResourceConditionDialog condition={resourceFilter} onChange={(condition) => setResourceFilter(parseResourceFilter(condition)) } > // Rest of the code...
219-225
: Add error handling for resource fetchingThe component doesn't handle errors that might occur during resource fetching with
useFilteredResources
. Consider adding error state handling.- const { resources, isLoading } = useFilteredResources( + const { resources, isLoading, error } = useFilteredResources( workspaceId, environment.id, finalFilter, PAGE_SIZE, page * PAGE_SIZE, ); + // Add after the early return for environment.resourceFilter == null + if (error) { + return ( + <Card> + <CardHeader className="flex flex-row items-center justify-between"> + <div className="space-y-1"> + <CardTitle>Resources</CardTitle> + <CardDescription> + Resources managed in this environment + </CardDescription> + </div> + <EditSelector environment={environment} /> + </CardHeader> + <CardContent> + <div className="text-red-400">Error loading resources: {error.message}</div> + </CardContent> + </Card> + ); + }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/resources/ResourcesPageContent.tsx
(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/resources/_components/EditSelector.tsx
(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/resources/page.tsx
(1 hunks)packages/api/src/router/environment-page/overview/router.ts
(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/resources/page.tsx
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{ts,tsx}`: **Note on Error Handling:** Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error...
**/*.{ts,tsx}
: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
packages/api/src/router/environment-page/overview/router.ts
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/resources/_components/EditSelector.tsx
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/resources/ResourcesPageContent.tsx
🧬 Code Definitions (1)
packages/api/src/router/environment-page/overview/router.ts (3)
packages/api/src/router/environment-page/overview/deployment-stats.ts (1)
getDeploymentStats
(36-117)packages/api/src/router/environment-page/overview/version-distro.ts (1)
getVersionDistro
(16-67)packages/api/src/router/environment-page/overview/desired-version.ts (1)
getDesiredVersion
(7-104)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build (linux/amd64)
- GitHub Check: Typecheck
- GitHub Check: Lint
🔇 Additional comments (4)
packages/api/src/router/environment-page/overview/router.ts (3)
34-68
: Validate environment ownership against workspace
Even though we receive bothenvironmentId
andworkspaceId
as input, there is no explicit check ensuring that the fetched environment actually belongs to the provided workspace. This could lead to data cross-contamination (e.g., referencing an environment from another workspace).You could verify this logic by checking the relevant fields in the
environment
table (e.g., compareenvironment.workspaceId
against the suppliedworkspaceId
) and throwing an error if they don’t match. Would you like me to generate a shell script to scan the rest of the codebase for any environment-to-workspace consistency checks or relevant references?
35-44
: Add check for a non-existent environment
If no rows are returned from the database,environment
will beundefined
. Accessingenvironment.systemId
at line 44 will cause a runtime error.const environment = await ctx.db .select() .from(SCHEMA.environment) .where(eq(SCHEMA.environment.id, environmentId)) .then(takeFirst); + if (!environment) { + throw new Error(`Environment with id ${environmentId} not found`); + } const deployments = await ctx.db .select() .from(SCHEMA.deployment) .where(eq(SCHEMA.deployment.systemId, environment.systemId));
117-121
: Validate deployment existence
If.then(takeFirst)
returnsundefined
, usingdeployment.resourceFilter
at lines 130–135 leads to a crash.const deploymentPromise = ctx.db .select() .from(SCHEMA.deployment) .where(eq(SCHEMA.deployment.id, deploymentId)) .then(takeFirst); + if (!deployment) { + throw new Error(`Deployment with id ${deploymentId} not found`); + } const [environment, deployment] = await Promise.all([ envPromise, deploymentPromise, ]);apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/resources/ResourcesPageContent.tsx (1)
466-466
:❓ Verification inconclusive
Add resourceStatus prop to ResourceCard component
The
ResourceCard
component likely expects a resourceStatus prop based on its definition, but it's not being passed.
🏁 Script executed:
#!/bin/bash # Check if ResourceCard accepts a resourceStatus prop ast-grep --pattern $'const ResourceCard: React.FC<{ resource: $_; resourceStatus$_: $_; }>' # Look up the component implementation to confirm rg --type typescript "export const ResourceCard.*?:.*?ResourceStatus" -A 3Length of output: 210
Below is a new set of shell scripts to further verify whether the
ResourceCard
component’s type definition actually expects aresourceStatus
prop. Please run these commands manually to confirm the component’s definition before addressing the review comment.#!/bin/bash # 1. List all files with "ResourceCard" in their name to locate the component definition. fd -i "ResourceCard" . # 2. Search for the ResourceCard component definition in the repository. rg -n "const ResourceCard:" . # 3. Additionally, search the entire codebase for any mention of "resourceStatus" to see if it is defined or used in a prop definition. rg -n "resourceStatus" .Next Steps:
• Verify that theResourceCard
component’s type (or props interface) indeed includes aresourceStatus
prop.
• If it is defined but not passed here, update the usage atapps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/resources/ResourcesPageContent.tsx
(line 466) accordingly.
• Otherwise, confirm if the review comment needs to be updated or removed.Please run the above commands and review the outputs. Then, adjust the code or the review comment as necessary.
const tokens = value.split(":"); | ||
|
||
const kind = tokens.at(1); | ||
if (kind == null) return; | ||
const trimmedKind = kind.trim(); | ||
if (trimmedKind.length === 0) return; | ||
|
||
handleFilterDropdownChange( | ||
trimmedKind, | ||
ResourceFilterType.Kind, | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add validation for version token in filter dropdown handler
The current implementation only verifies the presence of the kind token but not the version token.
const tokens = value.split(":");
- const kind = tokens.at(1);
- if (kind == null) return;
+ const version = tokens.at(0);
+ const kind = tokens.at(1);
+ if (version == null || kind == null) return;
+
const trimmedKind = kind.trim();
if (trimmedKind.length === 0) return;
handleFilterDropdownChange(
trimmedKind,
ResourceFilterType.Kind,
);
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
const tokens = value.split(":"); | |
const kind = tokens.at(1); | |
if (kind == null) return; | |
const trimmedKind = kind.trim(); | |
if (trimmedKind.length === 0) return; | |
handleFilterDropdownChange( | |
trimmedKind, | |
ResourceFilterType.Kind, | |
); | |
const tokens = value.split(":"); | |
const version = tokens.at(0); | |
const kind = tokens.at(1); | |
if (version == null || kind == null) return; | |
const trimmedKind = kind.trim(); | |
if (trimmedKind.length === 0) return; | |
handleFilterDropdownChange( | |
trimmedKind, | |
ResourceFilterType.Kind, | |
); |
...)/systems/[systemSlug]/(raw)/environments/[environmentId]/resources/ResourcesPageContent.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/resources/ResourcesPageContent.tsx (1)
467-469
: Consider adding resource status to ResourceCard.While you're correctly passing the resource object to the ResourceCard component, you might want to explicitly pass the status as a separate prop if the ResourceCard handles it separately.
{!isLoading && resources.map((resource) => ( - <ResourceCard key={resource.id} resource={resource} /> + <ResourceCard + key={resource.id} + resource={resource} + resourceStatus={resource.status} + /> ))}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/overview/page.tsx
(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/policies/PoliciesPageContent.tsx
(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/resources/ResourcesPageContent.tsx
(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/policies/PoliciesPageContent.tsx
- apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/overview/page.tsx
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{ts,tsx}`: **Note on Error Handling:** Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error...
**/*.{ts,tsx}
: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/resources/ResourcesPageContent.tsx
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Typecheck
- GitHub Check: Lint
- GitHub Check: build (linux/amd64)
🔇 Additional comments (10)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/environments/[environmentId]/resources/ResourcesPageContent.tsx (10)
56-66
: Good implementation of safe parsing with proper error handling.The
safeParseInt
function correctly handles potential parsing errors and includes proper validation of page boundaries. The conditional check(total > 0 ? page * PAGE_SIZE < total : true)
is particularly well-designed to handle the edge case when there are no resources.
68-78
: Well-implemented pagination hook with URL state.The
usePagination
hook elegantly manages pagination state by synchronizing it with the URL's search parameters. This approach enables bookmarking and sharing specific resource pages, which enhances user experience.
182-227
: Good filter composition from environment and local filters.The component correctly composes filters from both the environment's resourceFilter and the component's local resourceFilter state, using the isPresent utility to filter out null values. The pagination implementation with the custom hook is also well done.
252-257
: Well-structured resource grouping.Using lodash's groupBy for organizing resources by version and kind is efficient. The combined key pattern
t.version + ": " + t.kind
for resourcesByKind allows for more specific filtering options in the dropdown.
311-318
: Great dynamic calculation of resource health metrics.The implementation correctly calculates and displays the percentage of healthy resources, replacing the hardcoded values mentioned in previous reviews. The formatting with
Number(healthyPercentage).toFixed(0)
ensures a clean display without decimal places.
327-334
: Good conditional messaging based on resource health.The component displays appropriate messages based on the resource health status, providing helpful context to users ("Action required" vs "No issues detected").
343-351
: Effective status reporting for deploying resources.The component provides clear visual and textual feedback about ongoing deployments, with appropriate conditional messaging that enhances user understanding of the environment state.
390-400
: Add validation for version token in filter dropdown handler.The current implementation only verifies the presence of the kind token but not the version token.
const tokens = value.split(":"); - const kind = tokens.at(1); - if (kind == null) return; + const version = tokens.at(0); + const kind = tokens.at(1); + if (version == null || kind == null) return; + const trimmedKind = kind.trim(); if (trimmedKind.length === 0) return; handleFilterDropdownChange( trimmedKind, ResourceFilterType.Kind, );
479-494
: Good pagination info display with filtered indicator.The component intelligently displays different messages based on whether all resources are shown or just a subset, and includes a clear visual indicator when filters are applied. This provides excellent context for users.
495-512
: Well-implemented pagination controls.The Next/Previous buttons are properly disabled based on the current page and total resources, preventing users from navigating beyond available data.
Summary by CodeRabbit
New Features
EnvironmentOverviewPage
for a comprehensive view of environment details and statistics.EnvironmentTabs
for easy navigation between Overview, Deployments, Resources, and Policies.CopyEnvIdButton
for copying environment IDs to the clipboard.StatusBadge
component for visually representing deployment statuses.ResourceCard
for displaying resource details and aResourceTable
for structured resource presentation.DeploymentTelemetryTable
for displaying deployment telemetry data.TotalDeployments
,SuccessRate
,DeploymentFrequency
, andAverageDuration
.EditSelector
component for managing resource filters within environments.UI Improvements